Skip to content

Simplifying the flow #28010

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

Merged
merged 3 commits into from Nov 10, 2019
Merged

Simplifying the flow #28010

merged 3 commits into from Nov 10, 2019

Conversation

ghost
Copy link

@ghost ghost commented Nov 1, 2019

To the average programmer, looking at the original code shows plenty of branches that result in returning None. Because there are all contained in else statements, they can be somewhat hard to follow. For this reason, I put the bottom else statement out of the clause, and have it so that after each brach, if nothing returns, it would go to one return None statement.

Resolves SR-NNNN.

@CodaFi
Copy link
Contributor

CodaFi commented Nov 1, 2019

Looks good! Could you run clang-format really quickly to patch up the switch statement?

@ghost
Copy link
Author

ghost commented Nov 1, 2019

Did but clang format causes some other changes as well for some reason.

@owenv
Copy link
Contributor

owenv commented Nov 1, 2019

I recommend using the git clang-format tool. It will run clang-format on only the parts of the file you changed in a specific commit. Some parts of the compiler don't follow the style guidelines exactly but we want to avoid including unrelated style changes in PRs when possible

@ghost
Copy link
Author

ghost commented Nov 1, 2019

Done. thank you

@ghost
Copy link
Author

ghost commented Nov 3, 2019

Addressed

@ghost
Copy link
Author

ghost commented Nov 3, 2019

I managed to fix everything

@ghost ghost requested review from CodaFi and xwu November 3, 2019 21:53
Copy link
Collaborator

@xwu xwu left a comment

Choose a reason for hiding this comment

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

See previous comments. Otherwise, seems fine to me.

@ghost
Copy link
Author

ghost commented Nov 7, 2019

Fixed and rebased!

@ghost
Copy link
Author

ghost commented Nov 8, 2019

Done

@CodaFi
Copy link
Contributor

CodaFi commented Nov 8, 2019

That looks better

@swift-ci please smoke test

Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with us through the review!

@ghost
Copy link
Author

ghost commented Nov 8, 2019

No probs!

@CodaFi CodaFi merged commit 47aa977 into swiftlang:master Nov 10, 2019
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.

4 participants