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

schema builder fluency (fixes #303) #390

Conversation

JoshMcCullough
Copy link

Breaking Changes

Schema.Builder needs to return the subclassed builder instance rather than Builder<S> in order to allow for fluency while building schemas. See related issue.

Note: I also renamed ConstSchema.ConstSchemaBuilder to just Builder to match the other builders for consistency. I can pull this change back out if you don't want it. Since this PR is already a breaking change, I figured it made sense.

@JoshMcCullough
Copy link
Author

I'm trying to figure out how to instruct japicmp that it's okay that I renamed that class since this would be released as a new major...

@JoshMcCullough
Copy link
Author

Okay, updated the japicmp config to allow these changes for a major bump.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 91.474% when pulling 17fc6f7 on JoshMcCullough:jsm/schema-builder-fluency into da6e9fd on everit-org:master.

@erosb
Copy link
Contributor

erosb commented Nov 17, 2020

Hello @JoshMcCullough , thank you for your work on this. I'm afraid we won't break backward-compatibility and increment the major version number for the purpose of this single fix (and other potential breaking changes are not under discussion right now).

@JoshMcCullough
Copy link
Author

JoshMcCullough commented Nov 18, 2020

@erosb I understand the hesitance. And I can work with it using my own fork. I will just point out that creating a fluent schema as it is now is possible. But a general user would be confused if they tried it. I hope you'll bring this in at some point. Version numbers are just numbers, after all, and that's the point of semantic versioning. I would understand if you wanted to wait before doing so, but hope it's on your roadmap.

@erosb
Copy link
Contributor

erosb commented Nov 18, 2020

Yes, I thought about the same fix years ago. Thanks for the PR. Let's leave it open for now, and we will merge it when 2.0 is on the line.

@JoshMcCullough
Copy link
Author

Opened new PR: #465

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.

None yet

3 participants