Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SCHEMA_GENERATION_CLASS to settings, Allow NinjaGenerateJsonSchema to be overridden #1047

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Eittipat
Copy link

@Eittipat Eittipat commented Jan 13, 2024

Hi,

This pull request allow NinjaGenerateJsonSchema to be changed via settings

Usecase
I encounter a problem like #965.
while waiting for bug to be resolved. I have a workaround solution by custom behaviour of get_defs_ref inside NinjaGenerateJsonSchema

Allowing NinjaGenerateJsonSchema to be changed via settings enable clean solution to do this.
(I don't want to do monkey patch)

@vitalik
Copy link
Owner

vitalik commented Jan 14, 2024

Hi @Eittipat

Thank you for PR - could you please add at at least one test case for this and documentaition ?

Thank you

@Eittipat
Copy link
Author

@vitalik , Test, Guide and Doc added.

@vitalik
Copy link
Owner

vitalik commented Jan 15, 2024

Hi @Eittipat

thank you

I was wondering #965 should be addressed in the core ? Could you share your implementation to overcome duplicated schema names in different modules ?

@vitalik
Copy link
Owner

vitalik commented Jan 15, 2024

I assume you doing something like schema name in json becomes a combination of module-name + class name ?

I think the ideal could be that name is extened with a counter if duplicate is created (FooSchema, FooSchema2, FooSchema3 ...)

@Eittipat
Copy link
Author

Eittipat commented Jan 15, 2024

Yes, I think #965 should be addressed in the core too, However It is difference concern for this task. This pull request just add option to customize schema generator class which open opportunity to do something more.

Fixing #965 in the core need to be more careful (unlike my workaround that just good enough for my project). That's why I just propose this settings instead.

Of Course, I will continue looking for the way to fixing #965 with more cleaner and better solution. If I found some. I will open another pull request for sure.

Now, My implementation is very simple, It produce schema name as module_name + class_name.

class CustomNinjaGenerateJsonSchema(NinjaGenerateJsonSchema):

    def get_defs_ref(self, core_mode_ref: CoreModeRef) -> DefsRef:
        module_qualname_occurrence_mode = super().get_defs_ref(core_mode_ref)
        name_choices = self._prioritized_defsref_choices[module_qualname_occurrence_mode]
        name_choices.pop(0)
        name_choices.pop(0)
        self._prioritized_defsref_choices[module_qualname_occurrence_mode] = name_choices
        return module_qualname_occurrence_mode

Also, as your suggested, I think naming schema based on class_name + counter is more human readable but lead to be more confusing.

For example, when looking at FooSchema_2, I may be doubt where that name come from? I never had this class name before.

I'm open to discuss more about this topic, but again fixing #965 is not this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants