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.

Sign up for GitHub

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

Jump to bottom

Exhaustive case near and far #9656

Merged
merged 3 commits into from
Oct 30, 2020
Merged

Exhaustive case near and far #9656

merged 3 commits into from
Oct 30, 2020

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Aug 3, 2020 •
edited
Loading

This PR aims to be a small followup to #8424, it:

  • Uses exhaustive case statements in few places throughout the stdlib
  • Drops empty else # go on / ignore / nothing to do branches
  • Removes deprecated Log::Severity::Warning

@Sija
Copy link
Contributor Author

Sija commented Aug 3, 2020

I was thinking about opening another PR just for dropping Log::Severity::Warning, but it's so small that I'm not sure it's worth the hassle... I can do it if preferred though.

@Sija Sija force-pushed the exhaustive-case-near-and-far branch 2 times, most recently from e2a27a7 to 1767b62 Compare August 3, 2020 20:11
@Sija
Copy link
Contributor Author

Sija commented Aug 10, 2020 •
edited
Loading

Bump 🏓

@Sija
Copy link
Contributor Author

Sija commented Aug 24, 2020

Ping /cc @asterite @bcardiff @straight-shoota

asterite
asterite approved these changes Aug 24, 2020
View reviewed changes
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

@Sija
Copy link
Contributor Author

Sija commented Aug 30, 2020

Second approval any1? /cc @bcardiff @straight-shoota @waj @jhass

@Sija
Copy link
Contributor Author

Sija commented Sep 25, 2020

Yo, could this be merged or sth?

@asterite
Copy link
Member

It needs a second review.

@Sija
Copy link
Contributor Author

Sija commented Sep 25, 2020

@asterite so when this will happen?

@asterite
Copy link
Member

I don't know. But it doesn't matter, because until 1.0 is released whether this PR is merged or not doesn't change anything. In fact, even after it's merged it dosent produce visible changes.

@Sija
Copy link
Contributor Author

Sija commented Sep 25, 2020

@asterite That's the point, this PR doesn't bring any substantial changes so why does it takes almost 2 months already to get it through?

@Sija Sija force-pushed the exhaustive-case-near-and-far branch from 1767b62 to 906ef37 Compare October 28, 2020 22:41
@Sija
Copy link
Contributor Author

Sija commented Oct 28, 2020

Is there anything holding this back? /cc @straight-shoota @jhass @bcardiff @waj

@asterite asterite added this to the 1.0.0 milestone Oct 30, 2020
@asterite asterite merged commit da7039c into crystal-lang:master Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota approved these changes

@asterite asterite asterite approved these changes

Assignees
No one assigned
Labels
kind:refactor
Projects
None yet
Milestone
0.36.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Sija @asterite @straight-shoota