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

when vs if clarification #227

Merged
merged 2 commits into from
Apr 17, 2021
Merged

when vs if clarification #227

merged 2 commits into from
Apr 17, 2021

Conversation

holyjak
Copy link
Contributor

@holyjak holyjak commented Apr 9, 2021

The original "Use when instead of (if …​ (do …​))" made me skip it because I did not have a do in my code, only (if cond something...). So I want to clarify that it applies to all single-branch ifs, no matter whether they use do or not.

The original "Use when instead of (if …​ (do …​))" made me skip it because I did not have a `do` in my code, only `(if cond something...)`. So I want to clarify that it applies to all single-branch ifs, no matter whether they use do or not.
@seancorfield
Copy link
Collaborator

I'm strongly in favor of this guideline but I'm not sure the change really clarifies it since it still shows only if/do - I think it should also add "or (if expr truthy)" or something similar.

@bbatsov
Copy link
Owner

bbatsov commented Apr 10, 2021

I agree - more examples would probably be better than rewording the guideline.

@holyjak
Copy link
Contributor Author

holyjak commented Apr 12, 2021

Added an example. I still think it is clearer with a little more explanatory text than the original guideline so I left it there. Feel free to remove if you disagree.

@seancorfield
Copy link
Collaborator

I'm good with that. @bbatsov ?

@bbatsov
Copy link
Owner

bbatsov commented Apr 17, 2021

Fine by me as well.

@seancorfield seancorfield merged commit 75644d0 into bbatsov:master Apr 17, 2021
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