sinatra / mustermann

your personal string matching expert
http://sinatrarb.com/mustermann/
MIT License
664 stars 63 forks source link

Fix ruby 3.0 keyword arguments in translator #128

Closed tconst closed 2 years ago

tconst commented 2 years ago

Removes ruby2_keywords added in #126 and clears up the related console warnings in AST::Translator. This may not work in ruby < 2.7, untested at time of PR open.

dentarg commented 2 years ago

Pretty sure we need mustermann to work in Ruby < 2.7

tconst commented 2 years ago

Passing locally in 2.6.10 through 3.1.2.

dentarg commented 2 years ago

This PR seems to lower the test coverage from 100% to 99.71%, see https://github.com/dentarg/mustermann/runs/7406755584?check_suite_focus=true#step:4:28

tconst commented 2 years ago

@dentarg without merging the coverage files, not sure the best way to cover the differing branches. I added :nocov: to the ruby < 3 branch for now.

dentarg commented 2 years ago

True

dentarg commented 2 years ago

The change here get rids of the warnings (warning: Skipping set of ruby2_keywords flag for translate (method accepts keywords or method does not accept argument splat)

Is this change correct though? @eregon @jeremyevans do you care to weigh in?

tconst commented 2 years ago

Another way to fix (the warnings) would be something like this

      def self.translate(*types, &block)
        Class.new(const_get(:NodeTranslator)) do
          register(*types)
          define_method(:translate, &block)

          if block.parameters.flatten.any?(:args)
            ruby2_keywords :translate
          end
        end
      end

but I think this is not really a good solution as it is tied directly to the arg naming coming in with the block.

eregon commented 2 years ago

This PR/change is incorrect at least on 2.7: https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html

What are the warnings, for which ruby2_keywords call?

eregon commented 2 years ago

I created https://github.com/sinatra/mustermann/pull/130 which I believe is the proper fix. I don't know the details of this codebase though, but ruby2_keywords should only be applied to methods delegating *args. From a quick look at callers of translate only that one seems to delegate.

jkowens commented 2 years ago

@tconst I appreciate you helping work through this issue!

torrocus commented 2 years ago

@jkowens When can we expect patch on rubygems? Thanks in advance.

jkowens commented 2 years ago

Released v2.0.1