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

ISSUE-462: Add rexml dependency for Ruby 3.0.0+ #467

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

fbuys
Copy link
Collaborator

@fbuys fbuys commented Oct 2, 2023

We add rexml as a runtime dependency for environments that make use of Ruby 3 and up.

See: https://stackoverflow.com/a/65480744
Issue: #462

rexml does not add any additional dependencies: https://rubygems.org/gems/rexml.

I did test that this works with Ruby 3.2.2 and Ruby 2.7.8.

Check list:

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

* [CHANGE] Fix test warning related to `cucumber_opts` declaration (by [@faisal][])
* [BUGFIX] Stop using long-deprecated MiniTest module name, removed in 5.19.0 (by [@faisal][])
* [CHANGE] Add rexml dependency for Ruby 3.0.0+ (by [@fbuys][])
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

Choose a reason for hiding this comment

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

Checks failing because https://docs.rubocop.org/rubocop/cops_gemspec.html#gemspecrubyversionglobalsusage -> https://rubystyle.guide/#no-ruby-version-in-the-gemspec

Is checking the Ruby version worth it? I believe Bundler should use the installation-provided rexml if we don't care about its version. If we do care about the rexml version then we should care about it regardless of Ruby version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey!
@JuanVqz I added myself just now thank you!

@faisal I removed the ruby version and rexml versions. From my tests with Ruby 2.7 and Ruby 3.2 it does not seem like we need to specify the rexml version.

Copy link
Collaborator

@nunosilva800 nunosilva800 left a comment

Choose a reason for hiding this comment

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

LGTM, just add you name to the bottom of changelog and we can get this merged.
Thanks!

@faisal faisal mentioned this pull request Oct 4, 2023
3 tasks
@fbuys fbuys force-pushed the fbuys/462-support-ruby-3-2-2 branch 3 times, most recently from 9ef5095 to 6ce85e2 Compare October 5, 2023 15:15
We add rexml as a runtime dependency for environments that make use of
Ruby 3 and up

See: https://stackoverflow.com/a/65480744
Issue: whitesmith#462
@fbuys fbuys force-pushed the fbuys/462-support-ruby-3-2-2 branch from 6ce85e2 to b4595cc Compare October 5, 2023 15:19
@fbuys fbuys requested review from faisal and JuanVqz October 5, 2023 15:19
Copy link
Contributor

@faisal faisal left a comment

Choose a reason for hiding this comment

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

LGTM. I ran the tests against Ruby 2.7, 3.0, 3.1, and 3.2, and it tested out cleanly for all.

Copy link
Contributor

@JuanVqz JuanVqz left a comment

Choose a reason for hiding this comment

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

@fbuys, thank you!

@fbuys
Copy link
Collaborator Author

fbuys commented Oct 12, 2023

@nunosilva800 can we merge?

@etagwerker etagwerker merged commit ebdeba5 into whitesmith:main Oct 17, 2023
1 check passed
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.

5 participants