-
-
Notifications
You must be signed in to change notification settings - Fork 113
✨ Support eval_gemfile #248
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 sits on top of #250, since that PR fixes CI, and adds more Ruby engines and versions to the matrix. |
spec/acceptance/eval_gemfile_spec.rb
Outdated
| end | ||
|
|
||
| def build_gemspec(path = ".") | ||
| Dir.mkdir("tmp/stage/#{path}") rescue nil |
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 was an existing pattern in the tests I merely copied.
spec/acceptance/eval_gemfile_spec.rb
Outdated
| end | ||
|
|
||
| def build_modular_gemfile | ||
| Dir.mkdir("tmp/stage/gemfiles") rescue nil |
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 was an existing pattern in the tests I merely copied.
|
ping @nickcharlton 🔶 |
I don't think so, see: https://github.com/thoughtbot/appraisal/actions/runs/10999060013, but it might be due to a change in Bundler 😢 |
|
Interesting. Actually there may only be a few minor failures. From the Ruby 3.4 build: The difference is there are spaces around the Also there is a semicolon before |
|
@nickcharlton I tihnk some of the failures were legit broken stuff. I've fixed pretty much everything (and done some other things too) in a new PR: |
|
@pboling thank you! |
- Support debug for debugging with Ruby >= 2.7 - Support byebug for debugging with Ruby < 2.7
…d matrix - all current jruby and truffleruby are compatible with Ruby 3.1 at minimum, thus are supported by latest bundler
- Use strip_heredoc uniformly in acceptance tests
- Differences in handling of whitespace between versions of Ruby are often seen as minor, and go unnoticed unless tests explicitly test white space as appraisal's test suite does.
|
Rebased on top of the PR that fixes the build (#250) down to Ruby 2.7. |
- Ruby < 2 compat
- Possibly due to bundler issue on MRI and JRuby engines - ruby/rubygems#8518
- Fixes #154
|
Rebased on my |
| desc "Run the given task for all appraisals" | ||
| task appraisal: "appraisal:all" | ||
| desc("Run the given task for all appraisals") | ||
| task(:appraisal => "appraisal:all") |
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.
Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
| task(:all) do | ||
| ARGV.shift | ||
| exec "bundle exec appraisal rake #{ARGV.join(' ')}" | ||
| exec("bundle exec appraisal rake #{ARGV.join(" ")}") |
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.
Style/StringLiteralsInInterpolation: Prefer single-quoted strings inside interpolations.
| exec "bundle exec appraisal #{appraisal.name} rake #{ARGV.join(' ')}" | ||
| warn("`rake appraisal:#{appraisal.name}` task is deprecated and will be removed soon. " \ | ||
| "Please use `appraisal #{appraisal.name} rake #{ARGV.join(" ")}`.") | ||
| exec("bundle exec appraisal #{appraisal.name} rake #{ARGV.join(" ")}") |
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.
Style/StringLiteralsInInterpolation: Prefer single-quoted strings inside interpolations.
Metrics/LineLength: Line is too long. [84/80]
| "Please use `appraisal #{appraisal.name} rake #{ARGV.join(' ')}`." | ||
| exec "bundle exec appraisal #{appraisal.name} rake #{ARGV.join(' ')}" | ||
| warn("`rake appraisal:#{appraisal.name}` task is deprecated and will be removed soon. " \ | ||
| "Please use `appraisal #{appraisal.name} rake #{ARGV.join(" ")}`.") |
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.
Style/StringLiteralsInInterpolation: Prefer single-quoted strings inside interpolations.
Metrics/LineLength: Line is too long. [83/80]
| warn "`rake appraisal:#{appraisal.name}` task is deprecated and will be removed soon. " + | ||
| "Please use `appraisal #{appraisal.name} rake #{ARGV.join(' ')}`." | ||
| exec "bundle exec appraisal #{appraisal.name} rake #{ARGV.join(' ')}" | ||
| warn("`rake appraisal:#{appraisal.name}` task is deprecated and will be removed soon. " \ |
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.
Metrics/LineLength: Line is too long. [103/80]
| end | ||
|
|
||
| # Appraisal needs to print Gemfiles in the oldest Ruby syntax that is supported by Appraisal. | ||
| # This means formatting Hashes as Rockets, until support for Ruby 1.8 is dropped. |
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.
Metrics/LineLength: Line is too long. [85/80]
| end | ||
| end | ||
|
|
||
| # Appraisal needs to print Gemfiles in the oldest Ruby syntax that is supported by Appraisal. |
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.
Metrics/LineLength: Line is too long. [97/80]
|
|
||
| if enclosing_object | ||
| "{ #{items.join(', ')} }" | ||
| "{ #{items.join(", ")} }" |
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.
Style/StringLiteralsInInterpolation: Prefer single-quoted strings inside interpolations.
| end | ||
|
|
||
| # Appraisal needs to print Gemfiles in the oldest Ruby syntax that is supported by Appraisal. | ||
| # Otherwise, a project would not be able to use Appraisal to test compatibility |
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.
Metrics/LineLength: Line is too long. [83/80]
| Gem::Version.create(Bundler::VERSION) > Gem::Version.create("2.4.22") | ||
| end | ||
|
|
||
| # Appraisal needs to print Gemfiles in the oldest Ruby syntax that is supported by Appraisal. |
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.
Metrics/LineLength: Line is too long. [97/80]
|
@nickcharlton ping! |
|
I haven't looked at this properly yet, because it's stacked on all of the other work. But once we break that out, we can come back to this and get it merged in! |
|
Now that I'm starting to use this more heavily, I've discovered major side benefits of
You can see how powerful they are here:
As an example I added the gem |
|
It is clear that the community needs these fixes. I am personally implementing this branch across dozens of major projects currently. Would Thoughtbot consider adding me as a maintainer? Part of my reluctance in splitting this into small PRs is the long intervals spent waiting for attention from existing maintainers on the two existing PRs I submitted (which grew over time, but started out pretty small). If the cycle time holds the split PRs may not all get merged for years, and the community will have fully splintered into hard coded dependencies on forks, which will invariably result in resistance to switching back to mainline and getting further upgrades (not least because git dependencies do not show up in many tools that check for out of date dependencies). This tool is too important to let it die, and it might make more sense to create an appraisal org, and hard fork the gem to appraisal2, so it is untethered from thoughtbot. Status quo is not working well. |
I do agree
Why not if it helps adding maintainers to the project. On the other hand it should not be difficult to add maintainers to the current project in thoughtbot org.
I don't think this is a good idea. I would prefer transferring appraisal gem to appraisal org rather than doing a hard fork to avoid multiplication of official repos. @nickcharlton let us know if we can do anything to get this PR merged. thank you! |
|
I agree that it could live on under Thoughtbot, but only just. I don't think Thoughtbot has the level of corporate interest in this project to sustain it. If they were aware of the issue and wanted to, something would have been done already. They've already effectively killed this project twice. First when it remained incompatible with GHA for years, I had to rip it out of projects I migrated to GHA from other CIs. Second, after they fixed it to work with GHA, they dropped support for old Rubies, which coupled with the general lack of development, meant my choices were to rip it out again, or fix it. It isn't clear Thoughtbot wants this project attached to them anymore aside from a historical byline. I think it is likely that the bulk of the company's interest lies outside of Ruby now... which is perfectly valid. They don't have to hold onto it forever. Perhaps it is time for thoughtbot to move the project to a discrete org. I'll go ahead and create one in case the ball moves in that direction, and I'll sent owner invites to invested parties. |
|
Also, thinking more about this, So to that end I'll fork this to the new org, and rename it to
🚀 |
|
For better or worse: @nickcharlton @n-rodriguez @deivid-rodriguez you've all been invited to the org, as the top three contributors to this project who had any involvement in the last several years. |
|
I just realized that another mental blocker I had for redoing this PR in small parts is I actually already did a significant additional amount of work on top of it, fixing many other issues.
All of this work is going to be in |




eval_gemfile#154Please help me support open source by sponsoring me - @pboling