Skip to content

Move Prism dependency to gemspec file from Gemfile#288

Closed
gemmaro wants to merge 1 commit intoruby:masterfrom
gemmaro:prism
Closed

Move Prism dependency to gemspec file from Gemfile#288
gemmaro wants to merge 1 commit intoruby:masterfrom
gemmaro:prism

Conversation

@gemmaro
Copy link
Contributor

@gemmaro gemmaro commented Jan 1, 2025

Using TypeProf as a library does not resolve Gemfile dependencies. In particular, TypeProf uses the start_code_units_column method introduced in Prism version 0.23.0, so it must be at least that version or later.

* Gemfile, typeprof.gemspec: Using TypeProf as a library does not resolve
Gemfile dependencies.  In particular, TypeProf uses the
start_code_units_column method introduced in Prism version 0.23.0, so it must
be at least that version or later.
@sinsoku
Copy link
Contributor

sinsoku commented Oct 6, 2025

@gemmaro
The test failed on my machine too.
Adding the dependency to typeprof.gemspec looks good, but I think we need to keep the dependency in the Gemfile.
This is because gemspec is commented out in the Gemfile, and the gemspec dependencies are not being loaded.
https://github.com/ruby/typeprof/blob/24cf28ed49a8e82d289e5638e092e10ba48007dd/Gemfile#L7C4-L7C11

My guess is that this is intentionally not being loaded to prevent the typeprof version from being included in Gemfile.lock.

@gemmaro
Copy link
Contributor Author

gemmaro commented Oct 6, 2025

@sinsoku I understand your point. Since TypeProf is a gem used as an application rather than a library, it makes sense to keep things as they are. I'll go ahead and close this then.

@gemmaro gemmaro closed this Oct 6, 2025
@gemmaro gemmaro deleted the prism branch October 6, 2025 23:52
@sinsoku
Copy link
Contributor

sinsoku commented Oct 9, 2025

@gemmaro
I'm sorry for the confusion. My previous comment wasn't clear enough.

I think adding prism to the gemspec is a good change. What I meant to suggest was:

  • Add spec.add_runtime_dependency "prism", ">= 1.4.0" to typeprof.gemspec
  • Keep gem "prism", ">= 1.4.0" in the Gemfile as is (no changes to Gemfile)

Having the dependency in both places ensures it works properly for both gem users and development environments.

Note: The prism version requirement was updated to ">= 1.4.0" in #312.

Would you like to reopen the PR with just the gemspec change?

@gemmaro
Copy link
Contributor Author

gemmaro commented Oct 10, 2025

Thank you for the clarification.
I’m happy to contribute any changes that would be beneficial for TypeProf.

Your suggested modification sounds good to me. However, before proceeding, I’d like to briefly ask the repository members about how they prefer to manage dependencies and respect their intention.

I’ll mention @mame to confirm -- sorry in advance for my noise -- whether the decision not to load the gemspec from the Gemfile was indeed as @sinsoku explained, and whether the proposed approach would be acceptable.

Also, thank you for sharing that the version requirement for Prism was updated. I appreciate it.

@mame
Copy link
Member

mame commented Nov 3, 2025

I'm so sorry for the extremely late reply!

I don't recall the reason why I avoided loading the gemspec in the Gemfile. I think it's probably fine to load it.
To be honest, I'm not very familiar with Gemfile conventions and have mostly been managing it through trial and error.

I did see this PR, but I was hesitant and let it sit because I wanted to avoid adding new dependencies if possible. However, we are already actually depending on it, there's no point in holding out anymore.

Therefore, I agree with the following changes:

  • Add spec.add_runtime_dependency "prism", ">= 1.4.0" to typeprof.gemspec
  • Add gemspec to the Gemfile

I feel terrible for letting this sit for so long, but would you be willing to create a new PR with these changes?
Alternatively, I'm planning to do a release around tomorrow evening, so I can also take care of it myself.

@gemmaro
Copy link
Contributor Author

gemmaro commented Nov 3, 2025

@mame Thank you for your response!
No worries about the delay at all - I completely understand. I really appreciate you taking the time to review this.
I've created a new pull request #347 with the changes you suggested.

Please let me know if you'd like any adjustments to the PR. Thank you again for maintaining this project!

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.

3 participants