-
-
Notifications
You must be signed in to change notification settings - Fork 531
Remove non-idiomatic exclamation (!) suffix from method names
#1801
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
Conversation
|
This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested. If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos. For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping |
|
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
| end | ||
|
|
||
| # Popcorn is 🍿 | ||
| def claim_free_popcorn! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bang indicates dangerous, as in may raise an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All methods may raise an exception. I don't believe the bang has a definitive connection with raising errors.
Many Rails methods take this differentiation in behaviour when compared to their non-bang counterpart. But even the non-bang methods raise errors under the right circumstances. E.g. ActiveRecord's save vs save! can both raise an exception if the Database goes down or the Ruby process runs out of memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, those are both exceptional behaviors, though. And exceptions are for exceptional behavior.
Contrary to some of our exercises, even. This exercise, even, as it is arguably using the exception as flow control. I am aware of this, yet the discussions happen in mentoring (at least I discuss this during mentoring).
|
|
||
| # Popcorn is 🍿 | ||
| def claim_free_popcorn! | ||
| def claim_free_popcorn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be very surprising to have an assertion come up, but the tests require that an exception is raised, so the bang is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, all Ruby methods can raise errors. It is not surprising, but it is exceptional!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
| element | ||
| end | ||
|
|
||
| def reverse! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there is no reverse without, this does mean "change in place". If the tests verify that it is a mutation, then the bang should stand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bang (!) does not mean “destructive” nor lack of it mean non destructive either.
There are many methods in the Ruby standard library that mutate the receiver and do not include an exclamation mark in the name. For example, String#concat, Array#pop, Hash#clear.
These are idiomatic because they have no counterpart method.
| end | ||
|
|
||
| def test_list_in_place_reverse! | ||
| def test_list_in_place_reverse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the test name does not necessarily need the exclamation point. A not in place reverse method could also be tested. The bang here was only a literal reflection of the method name, without a way to fully indicate that.
This could be removed. The method itself will be indicated in the actual method use in the test.
Context
Several learner-facing exercises require exclamaition-suffixed methods:
issue_pass!,revoke_pass!,claim_free_popcorn!,update!,reverse!These are non-idiomatic uses of such names in the Ruby language, according to Matz, the creator of Ruby:Thus, in idiomatic Ruby, to suffix a method name with an exclamation (bang), there needs to be a corresponding method similarly named without an exclamation. However, no such corresponding method exists in these examples.
Change
Rename those APIs to their non-bang forms across stubs, unit tests, and exemplar implementations (
issue_pass,revoke_pass,claim_free_popcorn,update, reverse). The implementations themselves remain behaviourally identical; only their public signatures and matching expectations were adjusted to drop the non-idiomatic suffix.Consequences
Learners can now work with idiomatic Ruby naming. Clarity is improved without altering the core exercises.