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

Rubocop name cops #440

Merged
merged 9 commits into from Mar 14, 2020
Merged

Rubocop name cops #440

merged 9 commits into from Mar 14, 2020

Conversation

hainesr
Copy link
Member

@hainesr hainesr commented Feb 20, 2020

This PR goes through and cleans up rubocop naming cops that don't affect the API.

There were many variables/parameters/etc named in camelCase style, possibly a hangover from the early days of this library and its Java-like roots. This PR renames them to ruby standard snake_case style.

Most names have been translated as simply as possible - e.g. entryName to entry_name - but some have been simplified as well - e.g. aProc to proc. There are no changes to functionality in this PR; any other changes, such as formatting, have only been done to conform to our existing rubocop standards.

@coveralls
Copy link

coveralls commented Feb 20, 2020

Coverage Status

Coverage remained the same at 95.767% when pulling ce17c57 on hainesr:rubocop-names into 8d91d00 on rubyzip:master.

Copy link
Member

@jdleesmiller jdleesmiller left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on!

I have now read through. It may look like a lot of comments, but they are for a small number of repeated things.

lib/zip/file.rb Outdated Show resolved Hide resolved
lib/zip/file.rb Outdated Show resolved Hide resolved
lib/zip/file.rb Outdated Show resolved Hide resolved
lib/zip/filesystem.rb Outdated Show resolved Hide resolved
lib/zip/filesystem.rb Outdated Show resolved Hide resolved
test/settings_test.rb Outdated Show resolved Hide resolved
test/filesystem/file_nonmutating_test.rb Show resolved Hide resolved
test/file_test.rb Show resolved Hide resolved
test/file_test.rb Show resolved Hide resolved
test/file_test.rb Show resolved Hide resolved
@hainesr
Copy link
Member Author

hainesr commented Feb 29, 2020

I've done the proc -> a_proc changes. You were definitely right about it being potentially dangerous, and I'm surprised that Rubocop doesn't have an opinion on it, like it does for open...

Copy link
Member

@jdleesmiller jdleesmiller left a comment

Choose a reason for hiding this comment

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

OK, thanks. All looks good.

On the changes to strings / file names: in general, changing both tests and code at the same time makes me a bit nervous, so I'd lean toward making the minimal set of changes possible to the tests. However, I don't see any particular problems here; it seems unlikely that the case of the the files in the tests is a significant factor in the test suite coverage.

@hainesr
Copy link
Member Author

hainesr commented Mar 2, 2020

Thanks. Point taken about changing code and tests together; I agree. But as you say these changes are fairly safe in this instance...

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

3 participants