Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Update templates #6121

Closed

Conversation

friederbluemle
Copy link

@friederbluemle friederbluemle commented Oct 24, 2017

This fixes several RuboCop warnings such as Prefer single-quoted strings when you don't need string interpolation or special symbols. (Style/StringLiterals), as well as markdownlint warnings related to long lines. Line length should be limited to 80 chars according to MD013.

Bundler also suggests to use single-quoted strings in Gemfile right on the frontpage.

What was the end-user problem that led to this PR?

The problem was that the default files generated by bundle init and bundle gem <newgem> were causing RuboCop warnings (default configuration)

What was your diagnosis of the problem?

My diagnosis was that the template files contained the described issues.

What is your fix for the problem, implemented in this PR?

My fix was to fix the RuboCop/Lint warnings.

Why did you choose this fix out of the possible options?

I chose this fix because there is no other fix (except using custom RuboCop rules or disabling them).

@ghost
Copy link

ghost commented Oct 24, 2017

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

@friederbluemle
Copy link
Author

Oh wow, Travis is reporting this:
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

I'm not exactly sure why, but even if bundler uses custom rules for their ruby code, the generated templates (for use in other projects), should be able to pass the default RuboCop rules, i.e. use single quotes unless you need string interpolation.

Does anyone have suggestions how to achieve this? I assume bundler is using the custom rule for a reason.

@colby-swandale
Copy link
Member

colby-swandale commented Oct 25, 2017

You will need configure Rubocop to skip the Style/StringLiterals check for those specific files to pass CI

edit: see https://github.com/bbatsov/rubocop/blob/master/manual/configuration.md#includingexcluding-files

@@ -23,7 +23,7 @@ include:
Examples of unacceptable behavior by participants include:

* The use of sexualized language or imagery and unwelcome sexual attention or
advances
advances
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need 2 spaces?

Copy link
Author

Choose a reason for hiding this comment

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

It needs two spaces because it's a continuation of the line above. Otherwise, some markdown implementation might interpret it as two separate lists (separated by the advances line). See these warnings:

CODE_OF_CONDUCT.md:25: MD032 Lists should be surrounded by blank lines
CODE_OF_CONDUCT.md:27: MD032 Lists should be surrounded by blank lines

Check a few lines below (the last two bullet points) - they have the two spaces as well.

@friederbluemle
Copy link
Author

@colby-swandale Thanks for the comments! I'll try to amend the commit to make CI pass now!

@friederbluemle friederbluemle force-pushed the update-templates branch 2 times, most recently from 58d3a5a to e7bb3cb Compare October 25, 2017 03:49
@friederbluemle
Copy link
Author

Hmm okay, RuboCop is passing now, but there is a different CI error. I will check later how to fix it.

@bundlerbot
Copy link
Collaborator

☔ The latest upstream changes (presumably #6123) made this pull request unmergeable. Please resolve the merge conflicts.

@friederbluemle friederbluemle force-pushed the update-templates branch 2 times, most recently from 94e819d to d155ad3 Compare October 30, 2017 09:51
@colby-swandale
Copy link
Member

ping @friederbluemle

@friederbluemle
Copy link
Author

Sorry for the delay @colby-swandale - I'll work on it this weekend!

@bundlerbot
Copy link
Collaborator

☔ The latest upstream changes (presumably #6201) made this pull request unmergeable. Please resolve the merge conflicts.

@friederbluemle
Copy link
Author

@colby-swandale Again sorry for the super delayed response. I rebased my branch and made it mergeable again. However, Travis is still failing. I ran the tests locally (rake spec) and this is what I'm getting:

Finished in 24 minutes 53 seconds (files took 1.49 seconds to load)
2327 examples, 20 failures, 1 pending

Failed examples:

rspec ./spec/bundler/gem_helper_spec.rb:150 # Bundler::GemHelper gem management #build_gem when build was successful creates .gem file
rspec ./spec/bundler/gem_helper_spec.rb:160 # Bundler::GemHelper gem management #install_gem when installation was successful gem is installed
rspec ./spec/bundler/gem_helper_spec.rb:209 # Bundler::GemHelper gem management rake release fails when there are unstaged files
rspec ./spec/bundler/gem_helper_spec.rb:214 # Bundler::GemHelper gem management rake release fails when there are uncommitted files
rspec ./spec/bundler/gem_helper_spec.rb:235 # Bundler::GemHelper gem management rake release succeeds on releasing
rspec ./spec/bundler/gem_helper_spec.rb:246 # Bundler::GemHelper gem management rake release succeeds even if tag already exists
rspec ./spec/commands/install_spec.rb:515 # bundle install with gem sources after installing with --standalone includes the standalone path
rspec ./spec/commands/newgem_spec.rb:309 # bundle gem gem naming with underscore requires the version file
rspec ./spec/commands/newgem_spec.rb:346 # bundle gem gem naming with underscore --exe parameter set requires 'test-gem'
rspec ./spec/commands/newgem_spec.rb:362 # bundle gem gem naming with underscore --bin parameter set requires 'test-gem'
rspec ./spec/commands/newgem_spec.rb:402 # bundle gem gem naming with underscore --test parameter set to rspec requires 'test-gem'
rspec ./spec/commands/newgem_spec.rb:458 # bundle gem gem naming with underscore --test parameter set to minitest requires 'test-gem'
rspec ./spec/commands/newgem_spec.rb:462 # bundle gem gem naming with underscore --test parameter set to minitest requires 'minitest_helper'
rspec ./spec/commands/newgem_spec.rb:598 # bundle gem gem naming with dashed requires the version file
rspec ./spec/commands/newgem_spec.rb:631 # bundle gem gem naming with dashed --bin parameter set requires 'test/gem'
rspec ./spec/commands/newgem_spec.rb:665 # bundle gem gem naming with dashed --test parameter set to rspec requires 'test/gem'
rspec ./spec/commands/newgem_spec.rb:699 # bundle gem gem naming with dashed --test parameter set to minitest requires 'test/gem'
rspec ./spec/commands/newgem_spec.rb:703 # bundle gem gem naming with dashed --test parameter set to minitest requires 'test_helper'
rspec ./spec/commands/newgem_spec.rb:755 # bundle gem gem naming with dashed --ext parameter set includes rake-compiler
rspec ./spec/commands/newgem_spec.rb:844 # bundle gem on first run asks about test framework

The failures are definitely caused by my changes, as the master branch passes. Would you have a tip where I could start looking? Is it possible to only re-run the failed (or specific) tests, rather than the entire test suite? Thanks!

@segiddins
Copy link
Member

Is it possible to only re-run the failed (or specific) tests, rather than the entire test suite? Thanks!

bin/rspec --only-failures should do the trick!

@segiddins
Copy link
Member

Looking at the test failures, it looks like a lot of the expectations just need to be updated to expect the files to use single quotes

@friederbluemle
Copy link
Author

Thanks a lot @segiddins - I managed to fix and successfully run all tests locally. There are only a few remaining errors with certain environment combinations now: https://travis-ci.org/bundler/bundler/builds/334670926 I will look into this later.

@segiddins
Copy link
Member

Don't worry about the 2.5.0 failures, we've got a fix in the works for that already.

@friederbluemle
Copy link
Author

@segiddins Okay sure thanks! I'll just rebase once the fixes are in 👍
Btw, I'm also using 2.5.0 locally.

@friederbluemle friederbluemle force-pushed the update-templates branch 2 times, most recently from ef5ec9d to 87bee46 Compare January 31, 2018 07:31
This fixes several rubocop and linter warnings, such as prefer usage of
single-quoted strings when string interpolation is no needed, and lines
that are too long.
@deivid-rodriguez
Copy link
Member

For what it's worth, there's an analysis about usage of the Style/StringLiterals cop in the most popular ruby projects here. Even though rubocop uses single_quotes as the default, it turns out that most of the most popular ruby projects tweak the config to use double_quotes.

The bundler case would be a little weird because while it uses double_quotes internally, it will (after this PR) promote usage of single_quotes on new gems.

@hsbt
Copy link
Member

hsbt commented Feb 1, 2018

Is it still a necessity?

When the rubocop changed single_quotes to double_quotes by default. Should we revert this pull request? It's no make sense.

This is out of scope of bundler.

@indirect
Copy link
Member

indirect commented Feb 1, 2018

Thanks for the discussion, everyone!

@hsbt it seems like Rubocop is not going to change their default, at least not right away, so we will need to pick a style and stick with it for ourselves.

@deivid-rodriguez thank you for researching the project defaults, and I'm excited to see if Rubocop will change the default. :)

@friederbluemle I super appreciate your work writing this PR!

I love that you're doing the work to make sure that new gems will pass a linter right away after they've been generated. That said, Bundler (along with the other most popular Ruby projects and libraries) have all adopted double-quotes as their standard quotes. It was super observant to catch that we have single quotes on the Bundler homepage, and we should definitely change those to match the rest of the project. 👍

Because Bundler has chosen double quotes, I think we should resolve the underlying issue of new gems failing Rubocop by adding a rubocop config to generated gems that enables the double-quote rule.

For our markdown files, it's great to see that you care about linting them as well. This PR will update the markdown files to pass mdl, but future PRs might undo your work and cause them to fail linting in the future. To prevent that, we'll need to set up mdl as part of our CI process, just like Rubocop is today. If you're interested in doing that, we'd be happy to answer any questions you have, either here or in the Bundler slack.

On a slightly related note, the reason that I personally didn't want to enforce 80-char lines in the past is that inline markdown links get pretty silly-looking with an 80-char max. To fix that, I think we should probably migrate our markdown documentation files to use named links. For example, [the bundle install man page](http://bundler.io/man/bundle-install.1.html) might instead become

blah blah [the bundle install man page][install-man] blah blah

[install-man]: http://bundler.io/man/bundle-install.1.html

I don't think rewriting the links in our existing markdown is a blocker to accepting this PR, though. Thanks again for all your work on this!

@bundlerbot
Copy link
Collaborator

☔ The latest upstream changes (presumably #6503) made this pull request unmergeable. Please resolve the merge conflicts.

@hsbt
Copy link
Member

hsbt commented Mar 26, 2020

@friederbluemle Bundler repo was merged into rubygems. If you still interested in this PR, Can you re-open this on https://github.com/rubygems/rubygems? Thanks.

@hsbt hsbt closed this Mar 26, 2020
@friederbluemle friederbluemle deleted the update-templates branch March 26, 2020 22:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants