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

Update Ruby versions on Travis CI #30

Merged
merged 7 commits into from Jan 7, 2020
Merged

Update Ruby versions on Travis CI #30

merged 7 commits into from Jan 7, 2020

Conversation

aried3r
Copy link
Contributor

@aried3r aried3r commented Nov 29, 2019

Let's see if this just works :)

@coveralls
Copy link

coveralls commented Nov 29, 2019

Coverage Status

Coverage increased (+0.2%) to 99.779% when pulling 621b81b on aried3r:patch-1 into 26e0cb9 on simonc:master.

@aried3r
Copy link
Contributor Author

aried3r commented Nov 29, 2019

Hmm, quite a long list of failures on Ruby 2.6. I'll try to have a look.

@simonc
Copy link
Owner

simonc commented Nov 29, 2019

Hi @aried3r!

Thanks for opening this PR. I haven't been very active on the project for a while but I can help get it working for Ruby 2.6.

If you have any question or if you want to have a pairing session I'm available (French timezone though ^^).

Cheers !

@aried3r
Copy link
Contributor Author

aried3r commented Dec 2, 2019

Ok, so some of the failures are indeed because of changes in Ruby 2.6.

Ruby 2.6 includes version 1.1 of the fileutils gem, which includes this change: ruby/fileutils#13. fileutils is now a stdgem (https://stdgems.org/), but I think that's unrelated. In this case, I'd just adopt the expected result if RUBY_VERSION >= 2.6, WDYT?

The errors related to missing children are because of the addition of Dir.children, however I haven't looked into it how exactly this gem is using this, directly or indirectly, especially since it fails on MemFs::Dir. Maybe you can shed some light on how to best fix this?

ruby/fileutils@d8c665c
ruby/fileutils@bc92c93

@simonc
Copy link
Owner

simonc commented Dec 3, 2019

@aried3r The condition on RUBY_VERSION seems to be a sensible choice. I don't see a more explicit solution for this issue.

Regarding the missing children method it's failing on MemFs::Dir because it's the class replacing the original Ruby Dir class when MemFs is enabled. I think the next step is to implement this method in MemFs::Dir.

The only interrogation I have is if we should condition this method's definition with if RUBY_VERSION >= 2.6 🤔

My pro argument is that it would prevent this method from being defined in earlier versions and thus prevent unwanted effects in users specs.

My con argument is that no other method was conditioned by the current Ruby version before so I'm not sure it'd make sense to do it in this specific occasion.

What's your opinion on that matter?

Copy link
Collaborator

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

.rubocop.yml: Style/AlignParameters has the wrong namespace - should be Layout
.rubocop.yml: Style/AlignParameters has the wrong namespace - should be Layout
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming
.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Style/MultilineOperationIndentation has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
obsolete parameter MaxLineLength (for Style/IfUnlessModifier) found in .rubocop.yml
`Style/IfUnlessModifier: MaxLineLength` has been removed. Use `Metrics/LineLength: Max` instead

Copy link
Collaborator

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

.rubocop.yml: Style/AlignParameters has the wrong namespace - should be Layout
.rubocop.yml: Style/AlignParameters has the wrong namespace - should be Layout
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming
.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Style/MultilineOperationIndentation has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
obsolete parameter MaxLineLength (for Style/IfUnlessModifier) found in .rubocop.yml
`Style/IfUnlessModifier: MaxLineLength` has been removed. Use `Metrics/LineLength: Max` instead

@aried3r
Copy link
Contributor Author

aried3r commented Jan 2, 2020

Hey! Seems like I pushed some changes but never followed up here. So I added the necessary methods and also made them dependent on the Ruby version installed. WDYT?

Copy link
Owner

@simonc simonc left a comment

Choose a reason for hiding this comment

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

@aried3r Thank you! I added some comments on the PR 😊

.travis.yml Outdated Show resolved Hide resolved
lib/memfs/dir.rb Outdated Show resolved Hide resolved
if Gem::Requirement.new('>= 2.6').satisfied_by?(Gem::Version.new(RUBY_VERSION))
describe '.children' do
it 'returns an array containing all of the filenames except for "." and ".."'\
'in this directory.' do
Copy link
Owner

Choose a reason for hiding this comment

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

Don't worry about line length for specs descriptions. I exluded the spec files from rubocop to avoid having this weird cuts 😄

end
else
describe '.children' do
it 'raises and error' do
Copy link
Owner

Choose a reason for hiding this comment

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

I think you meant an error 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

if Gem::Requirement.new('>= 2.6').satisfied_by?(Gem::Version.new(RUBY_VERSION))
describe '.children' do
it 'returns an array containing all of the filenames except for "." and ".."'\
'in this directory.' do
Copy link
Owner

Choose a reason for hiding this comment

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

Don't worry about line length for specs descriptions. I excluded the spec files from rubocop to avoid having this weird cuts 😄

Copy link
Collaborator

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

.rubocop.yml: Style/AlignParameters has the wrong namespace - should be Layout
.rubocop.yml: Style/AlignParameters has the wrong namespace - should be Layout
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming
.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Style/MultilineOperationIndentation has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
obsolete parameter MaxLineLength (for Style/IfUnlessModifier) found in .rubocop.yml
`Style/IfUnlessModifier: MaxLineLength` has been removed. Use `Metrics/LineLength: Max` instead

@aried3r
Copy link
Contributor Author

aried3r commented Jan 7, 2020

I knew I shouldn't have removed EOL Rubies and added Ruby 2.7 in one commit 😅

Now it gets interesting. In Ruby 2.7 File::extname returns . on non-Windows platforms. Do you want to handle this case (if Ruby >= 2.7 && !Windows (https://github.com/ruby/ruby/pull/2565/files#diff-01bda6262e55b3f376e1fd6ad669f78aR242))?

https://docs.ruby-lang.org/en/2.7.0/File.html#method-c-extname
https://docs.ruby-lang.org/en/2.7.0/NEWS.html
https://rubyreferences.github.io/rubychanges/2.7.html#fileextname-returns-a--string-at-a-name-ending-with-a-dot

Copy link
Collaborator

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

.rubocop.yml: Style/AlignParameters has the wrong namespace - should be Layout
.rubocop.yml: Style/AlignParameters has the wrong namespace - should be Layout
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming
.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Style/MultilineOperationIndentation has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
obsolete parameter MaxLineLength (for Style/IfUnlessModifier) found in .rubocop.yml
`Style/IfUnlessModifier: MaxLineLength` has been removed. Use `Metrics/LineLength: Max` instead

@aried3r
Copy link
Contributor Author

aried3r commented Jan 7, 2020

I added the Ruby 2.7 behavior of File::extname to the specs.

spec/memfs/file_spec.rb Show resolved Hide resolved
@simonc simonc merged commit d8e61ab into simonc:master Jan 7, 2020
@simonc
Copy link
Owner

simonc commented Jan 7, 2020

@aried3r Thanks for your work on this! ❤️

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.

None yet

4 participants