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

Resolves #1547: Make RemoveNullability virtual for extendability #1548

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

SamuelBerger
Copy link
Contributor

This allows to implement different logic e.g. treat JsonSchema.OneOf
to have multiple (non nullable) items instead of zero or one.

@lahma
Copy link
Collaborator

lahma commented Aug 7, 2022

Could you add a test case that demonstrates the type extension and overriding? This way we can ensure it won't be accidentally broken afterwards.

@SamuelBerger
Copy link
Contributor Author

Sure. But the current change is not changing any behaviour it just adds an extension point. While adding tests, what do you think of actually changing the current behaviour to only return the non nullable item if it is the only item in the collection?
Something like:

            if (schema.OneOf.Count == 1)
            {
                var singleItem = schema.OneOf.Single();
                if (!singleItem.IsNullable(SchemaType.JsonSchema))
                {
                    return singleItem;
                }
            }

            return schema;

or shorter

            return schema.OneOf.Count == 1 && schema.OneOf.Single() is {} singleItem && !singleItem.IsNullable(SchemaType.JsonSchema)
                ? singleItem
                : schema;

@lahma
Copy link
Collaborator

lahma commented Aug 7, 2022

My point was mostly about the train of thought "why is this virtual, let's remove the virtual modifier", and the next release will be broken. So mainly about making compilation level guarantees in code base that you suggested change will be respected.

@SamuelBerger
Copy link
Contributor Author

I see. In the same class the method GetOrGenerateTypeName is also virtual but never overriden in the project. Should I create a new test class like PublicApiGuardTests under NJsonSchema.CodeGeneration.Tests.csproj and override both methods to 'make them used'?

@lahma
Copy link
Collaborator

lahma commented Aug 7, 2022

That might be a good idea, adds barrier for changing the modifier. This is mostly nit-picking, but compilation safety adds some guarantees.

This allows to implement different logic e.g. treat JsonSchema.OneOf
to have multiple (non nullable) items instead of zero or one.
Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

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.

3 participants