Avoid line continuations without parentheses. #138
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This causes unexpected behaviour: The `not_to` spec will always pass, whilst the `to include` specs don't test for the presence of the command line, rather nothing. This is because Ruby treats the `include` as having no argument and the string below as a string literal. If we wrap in parentheses, the positive spec fails as the path is relative to the root partition. The solution here is to provide a full path when comparing `gemfile='...'`. This also fixes a broken expectation where `not_to` and `to` were flipped.
nickcharlton
force-pushed
the
nc-fix-acceptance-tests
branch
from
485e460
to
f18fee1
Compare
pablobm
approved these changes
Mar 13, 2018
| 'bundle install --gemfile=gemfiles/1.0.0.gemfile --jobs=0') | ||
| expect(output).not_to include( | ||
| 'bundle install --gemfile=gemfiles/1.0.0.gemfile --jobs=1') | ||
| "bundle install --gemfile='#{file('gemfiles/1.0.0.gemfile')}' --jobs=1" |
There was a problem hiding this comment.
I see Hound is not enabled, so this line (and similar ones above) is not being barked at for not ending on a comma. Is that ok by you?
There was a problem hiding this comment.
Unfortunately, that's an error under Ruby 1.8.7, which is still being run in CI. Removing the trailing comma fixes this.
One option here would be to first go about dropping support for 1.8.7. What do you think?
There was a problem hiding this comment.
Hm. One one hand, I'm for dropping 1.8.7, but on the other hand I don't find coding style a good enough reason for it.
I'm happy to leave it as is for now.
| expect(content_of 'gemfiles/1.0.0.gemfile.lock').not_to include | ||
| current_directory | ||
| expect(content_of("gemfiles/1.0.0.gemfile.lock")). | ||
| not_to include(current_directory) |
There was a problem hiding this comment.
I approve 👍 This code is 43.7% better without this ambiguity.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This causes unexpected behaviour: The
not_tospec will always pass,whilst the
to includespecs don't test for the presence of the commandline, rather nothing. This is because Ruby treats the
includeas having noargument and the string below as a string literal.
If we wrap in parentheses, the positive spec fails as the path is
relative to the root partition.
The solution here is to provide a full path when comparing
gemfile='...'. This also fixes a broken expectation wherenot_toandtowere flipped.