Skip to content
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

Generate Gemfile entry with version constraint #1244

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Dec 6, 2024

When generating a new application by loading the template over HTTP, specifying gem "suspenders" without a version constraint results in the resolution of an extremely outdated version (v0.2.4).

When omitting the --suspenders-main argument, include a version constraint to use a version that is at least 20240101.

When generating a new application by loading the template over HTTP,
specifying `gem "suspenders"` without a version constraint results in
the resolution of an extremely outdated version ([v0.2.4][]).

When omitting the `--suspenders-main` argument, include a version
constraint to use a version that is at least `20240101`.

[v0.2.4]: https://github.com/thoughtbot/suspenders/tree/v0.2.4
@seanpdoyle seanpdoyle force-pushed the include-gemfile-version branch from bc1ca6d to b181e38 Compare December 6, 2024 17:06
@@ -28,7 +28,7 @@ def apply_template!
if ARGV.include?("--suspenders-main")
gem "suspenders", github: "thoughtbot/suspenders", branch: "main"
else
gem "suspenders"
gem "suspenders", "> 20240101"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I tried (and failed) to see if I could do this dynamically by including Suspenders::Version in this file, and then doing something like this:

Suggested change
gem "suspenders", "> 20240101"
gem "suspenders", "> #{Suspenders::VERSION}"

The problem is that when this file is invoked, it's outside the context of Suspenders, so if Suspenders is not installed on the system, it will error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this is a Ruby file executed in its own environment without access to Bundler or other dependencies. I chose 20240101 because its a lower limit on the possible values (versions starting with the 2024-prefix), not necessarily because it's a valid version that has a release.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a Ruby file executed in its own environment without access to Bundler or other dependencies

I wonder if we could do something similar to the Rails reproduction scripts, but for now, this works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we could do something similar to the Rails reproduction scripts

While that might increase the runtime of rails new, that might be worthwhile, since it's (ideally) only executed once per project.

As an aside, I thought leaving off the version meant we'd download the latest version?

Ideally, that would be true. In practice, rails new would consistently install v0.2.4.

Comment on lines +15 to +21
test "generates Gemfile entry with version constraint" do
with_database "postgresql" do
run_generator
end

assert_file "Gemfile", /gem "suspenders", "> 20240101"/
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Were you able to get this to pass locally? I'm having some local dependency issues, so I can't verify this locally.

That being said, the file we're modifying in this commit is lib/install/web.rb, which is not a generator, but the application template.

This test is responsible for testing Suspenders ::Generators::Install:: WebGenerator, which doesn't install Suspenders.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, we don't have any coverage for lib/install/web.rb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants