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

Change nonsense return types to Nil: JSON and YAML #10622

Merged
merged 4 commits into from
Aug 2, 2021

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Apr 12, 2021

Strictly speaking, this is a backwards-incompatible change, but it should not affect any reasonable code. Attentive review is advised.

First I ran a tool to automatically insert return type restrictions based on what methods actually return in real usages. Then I picked out returns that are obviously purely accidental (due to implicitly returning the last expression) and can never be useful. So I change them to always be Nil.

How to review this:
The 1st commit exists only to provide a more useful diff in the 2nd commit.
Please look only at the 2nd commit (and not the overall diff) to see how this pull request changes actual return types that occur, into Nil.

@straight-shoota
Copy link
Member

I suppose the builder methods could even return self to allow method chaining. But that should be a separate change.

@oprypin

This comment has been minimized.

@straight-shoota straight-shoota added this to the 1.1.0 milestone Jun 7, 2021
@straight-shoota straight-shoota modified the milestones: 1.1.0, 1.2.0 Jun 17, 2021
@straight-shoota straight-shoota added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Aug 2, 2021
@straight-shoota straight-shoota merged commit d6db60a into crystal-lang:master Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:docs kind:refactor topic:compiler:parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants