revert adding psych as runtime dependency #6885
Conversation
|
hmm ok now I see why this was probably done. To be able to show a warning when there are duplicate keys in the yaml. No actually to be able to show the line_numbers... Not sure there is a way to get this information pre ruby 2.5. Changed it to simply not show the line numbers if |
|
I don't remember why this was needed to begin with. @rubocop-hq/rubocop-core Anyone knows? |
| @@ -196,14 +201,17 @@ def yaml_safe_load(yaml_code, filename) | |||
| if defined?(SafeYAML) && SafeYAML.respond_to?(:load) | |||
| SafeYAML.load(yaml_code, filename, | |||
| whitelisted_tags: %w[!ruby/regexp]) | |||
| else | |||
| # Ruby 2.6+ | |||
There was a problem hiding this comment.
Didn't you say this would be ok on 2.5+?
There was a problem hiding this comment.
I just reverted the exact comment that was removed in #6766
I only checked the start_line bit and saw that it is available since 2.5
There was a problem hiding this comment.
ok I checked and the kwargs used here where introduced in psych 3.1 (which is used since ruby 2.6)
| @@ -33,8 +33,10 @@ def check(yaml, &block) | |||
| key1 = key_a | |||
| key2 = key_b | |||
| end | |||
| expect(key1.start_line).to eq(0) | |||
| expect(key2.start_line).to eq(3) | |||
| if key1.respond_to?(:start_line) | |||
There was a problem hiding this comment.
We also have mechanism to run different specs on different Ruby versions. I think it'd be better to use it.
There was a problem hiding this comment.
can you point me to a spec which uses this? then I can adapt the PR
There was a problem hiding this comment.
See erb_new_arguments_spec.rb.
There was a problem hiding this comment.
hmm do I need to do anything more then set the rspec tags to make this work? it still seems to run all tests regardless which ruby version i'm using and therefore I get failing tests on old ruby version and errors on newer ones because of the changed output
There was a problem hiding this comment.
That's weird. I get the same results. I wonder if the RSpec functionality for specifying which Ruby version a certain scope is valid for has changed. This works:
diff --git a/spec/rubocop/yaml_duplication_checker_spec.rb b/spec/rubocop/yaml_duplication_checker_spec.rb
index 068270128..b6dc8548f 100644
--- a/spec/rubocop/yaml_duplication_checker_spec.rb
+++ b/spec/rubocop/yaml_duplication_checker_spec.rb
@@ -26,7 +26,7 @@ RSpec.describe RuboCop::YAMLDuplicationChecker do
include_examples 'call block'
- context '< Ruby 2.5', :ruby24 do
+ context '< Ruby 2.5', if: RUBY_VERSION >= '2.4' do
it 'calls block with keys' do
key1 = nil
key2 = nil
@@ -39,7 +39,7 @@ RSpec.describe RuboCop::YAMLDuplicationChecker do
end
end
- context '>= Ruby 2.5', :ruby25 do
+ context '>= Ruby 2.5', if: RUBY_VERSION >= '2.5' do
it 'calls block with keys' do
key1 = nil
key2 = nilso maybe we should change :ruby2x everywhere?
There was a problem hiding this comment.
ok I'll change my tests then to this syntax
There was a problem hiding this comment.
That's weird. I get the same results. I wonder if the RSpec functionality for specifying which Ruby version a certain scope is valid for has changed. This works:
There was a problem hiding this comment.
fixed for the tests I added/touched. For the other cases it doesn't seem to be a problem
There was a problem hiding this comment.
Setting the ruby version via :ruby25 actually just sets the ruby_version variable, which is used by the ProcessedSource, e.g. what parser version to be used. It's useful for cops specs, but here you are not testing a cop.
This reverts parts of #6766 by removing the psych runtime dependency again. This in term should fix #6781 and other issues regarding installation of psych from source on jruby.
From what I understand and is also written here the psych version is tightly coupled to the ruby version. Especially in this case I do not really see a good enough reason to explicitly require a newer version of psych.
Note I had to make a small change in the YAMLDuplicationChecker