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

Support Ruby 3 #38

Closed
wants to merge 3 commits into from
Closed

Support Ruby 3 #38

wants to merge 3 commits into from

Conversation

skalee
Copy link

@skalee skalee commented Jan 7, 2021

This pull request adds Ruby 3.0 compatibility, or at least makes tests passing locally. It's likely that other Relaton gems require changes too. I was experimenting with Ruby 3.0 compatibility on this gem only.

The only significant change is that double splat operator (**) is used when passing a hash to a method which expects keyword arguments rather than actual hash. This is required because Ruby 3.0 no longer decomposes such hashes implicitly (see https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/).

I am not sure if these are the only required changes, but they are enough to make tests passing.

Neither I'm sure if all these methods really should accept keyword arguments. Perhaps it'd be better to accept hash as a single positional argument. I wasn't analyzing that.

Apart from codebase itself, some dependencies also aren't compatible with Ruby 3.0 yet:

Ruby 3 separates positional and keyword arguments.  Hence, double splat
must be used to turn hashes into positional arguments.

See: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/
@@ -36,4 +36,5 @@ Gem::Specification.new do |spec|
spec.add_dependency "bibtex-ruby"
spec.add_dependency "iso639"
spec.add_dependency "nokogiri"
spec.add_dependency "rexml"
Copy link
Contributor

Choose a reason for hiding this comment

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

That's very strange -- we don't use REXML...?

Copy link
Author

@skalee skalee Jan 7, 2021

Choose a reason for hiding this comment

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

We do. Or more precisely, BibTeX-Ruby uses it (see inukshuk/bibtex-ruby#146).

Ruby Standard Library is in the process of moving into gems. REXML has been degraded to a bundled gem in Ruby 3.0, therefore has to be specified as dependency somewhere (see https://stdgems.org/rexml/). BibTeX-Ruby maintainer has decided to make it an optional dependency, so we got to specify it ourselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't write out to BibTeXML though, so that functionality is not needed here.

Copy link
Author

@skalee skalee Jan 8, 2021

Choose a reason for hiding this comment

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

Indeed. Initially it wasn't working without REXML, but it has been fixed in inukshuk/bibtex-ruby@4308f93. I'll remove this commit from the pull request.

Older versions do not work with Ruby 3.0.
@ronaldtse
Copy link
Contributor

@ronaldtse
Copy link
Contributor

And we need a rebase please, thanks!

@w00lf
Copy link

w00lf commented Apr 19, 2021

Hi there, @skalee we have an issue with relaton: https://github.com/metanorma/metanorma-standoc/pull/459/checks?check_run_id=2377920739

Failure/Error: bibitemxml = RelatonBib::BibliographicItem.new(RelatonBib::HashConverter::hash_to_bib(bib)).to_xml or next

     ArgumentError:
       wrong number of arguments (given 1, expected 0)
     # ./vendor/bundle/ruby/3.0.0/gems/relaton-bib-1.7.4/lib/relaton_bib/bibliographic_item.rb:192:in `initialize'

@andrew2net
Copy link
Contributor

@ronaldtse relaton fully supports Ruby 3.0 now. We don't need this PR anymore.

@ronaldtse
Copy link
Contributor

Thanks @andrew2net @skalee , closing.

@ronaldtse ronaldtse closed this Apr 19, 2021
@andrew2net
Copy link
Contributor

Hi there, @skalee we have an issue with relaton: https://github.com/metanorma/metanorma-standoc/pull/459/checks?check_run_id=2377920739

@w00lf the issue is in the ./lib/asciidoctor/standoc/cleanup_ref_dl.rb:11 line:

bibitemxml = RelatonBib::BibliographicItem.new(RelatonBib::HashConverter::hash_to_bib(bib)).to_xml or next

To work in Ruby 3.0 it should call RelatonBib::HashConverter::hash_to_bib(**bib)

@skalee
Copy link
Author

skalee commented Apr 19, 2021

I confirm that relaton-bib works in Ruby 3 now. Thanks!

@skalee skalee deleted the ruby-3 branch April 20, 2021 00:31
@w00lf
Copy link

w00lf commented Apr 20, 2021

Hi there, @skalee we have an issue with relaton: https://github.com/metanorma/metanorma-standoc/pull/459/checks?check_run_id=2377920739

@w00lf the issue is in the ./lib/asciidoctor/standoc/cleanup_ref_dl.rb:11 line:

bibitemxml = RelatonBib::BibliographicItem.new(RelatonBib::HashConverter::hash_to_bib(bib)).to_xml or next

To work in Ruby 3.0 it should call RelatonBib::HashConverter::hash_to_bib(**bib)

Tried to apply your hotfix but encountered a situation when specs fail on ruby version less than 2.7 with error:

 TypeError:
       hash key "id" is not a Symbol

It seems that ruby version less than 2.7 does not support string keys, if i try to use RelatonBib::HashConverter::hash_to_bib(**bib.symbolize_keys) i got an error in ruby 3.0: wrong number of arguments (given 1, expected 0)

@andrew2net
Copy link
Contributor

@w00lf sorry, it's my mistake. Try to replace the line

bibitemxml = RelatonBib::BibliographicItem.new(RelatonBib::HashConverter::hash_to_bib(bib)).to_xml or next

with this

bibitemxml = RelatonBib::BibliographicItem.from_hash(bib).to_xml

@w00lf
Copy link

w00lf commented Apr 20, 2021

@w00lf sorry, it's my mistake. Try to replace the line

bibitemxml = RelatonBib::BibliographicItem.new(RelatonBib::HashConverter::hash_to_bib(bib)).to_xml or next

with this

bibitemxml = RelatonBib::BibliographicItem.from_hash(bib).to_xml

That works, all tests are passing. Thanks!

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.

4 participants