separate test run for rubocop >= 0.87 #181
Conversation
.travis.yml
Outdated
| @@ -1,12 +1,14 @@ | |||
| language: ruby | |||
| before_install: | |||
| - rm ./Gemfile.lock | |||
| - rm ./gemfiles/Gemfile.*.lock | |||
There was a problem hiding this comment.
The CI step that removes lockfiles is confusing to me. I'm not sure why we even check in a lockfile if we don't use it in CI. @Edouard-chin I believe you introduced this two years ago. Do you still think this is a good idea?
I'd rather have something like dependabot bump our dependencies and always use the same lockfile.
There was a problem hiding this comment.
I can't recall a single other project I contribute to that checks in the lockfiles, which is why I opened #175
There was a problem hiding this comment.
I agree. I'll remove the lockfiles in this PR.
There was a problem hiding this comment.
Yeah, I think when you're deploying an application, it can make sense to have a lockfile floating around. We're not deploying/publishing a binary here, so it's likely providing little to no value.
There was a problem hiding this comment.
Few years ago the ruby community shifted to include the Gemfile.lock under VCS. It makes development of a library as well as bisecting much easier. Bundler explained it in more details in a blog https://bundler.io/blog/2018/01/17/making-gem-development-a-little-better.html
The CI step that removes lockfiles is confusing to me.
When CI runs we want to make sure that the library works on the most recent version of the gem. Though now that dependabot is a thing, I'm fine to use the same lockfile
There was a problem hiding this comment.
I agree. I'll remove the lockfiles in this PR.
Please put it back. Almost all shopify projects have the Gemfile.lock commited, and the ones that didn't should be updated. When bootrapping a gem nowaday, the Gemfile.lock isn't gitignored
There was a problem hiding this comment.
Why does it make sense to have a Gemfile.lock checked in that is not tested?
🤔
I also really don't want to get caught up in the arguments here... I'm trying to solve a different problem. I think we should leave the discussion about what to do with the lockfile in CI for @marcandre's issue #175.
For this PR, I'll keep the semantics that currently exist on master.
I'll keep Gemfile.lock in place so that it will be used by default on developer's computers, and add my two separate gemfiles for CI - I just wouldn't check in the lockfiles for these CI-specific gemfiles, and instead ignore them.
There was a problem hiding this comment.
I just wouldn't check in the lockfiles for these CI-specific gemfiles, and instead ignore them.
Yes that's the right thing to do
Why does it make sense to have a Gemfile.lock checked in that is not tested?
The bundler blog I shared explains it pretty well, but the TL;DR is that it makes contributing to the project easier
We want to test code paths specific to these new APIs while still also testing the old code paths for compatibility
|
Moved dropping support for ruby 2.4 into a separate PR #182 |
|
@marcandre I think you can now cherry-pick this onto your branch. The failing builds are the ones with the newer rubocop, as we would have expected. |
|
|
|
it was cherry-picked and will be merged with #179 |
This came up as a requirement in #179 (comment)