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

Improve rubocop settings #131

Merged
merged 1 commit into from
Apr 20, 2016
Merged

Improve rubocop settings #131

merged 1 commit into from
Apr 20, 2016

Conversation

gssbzn
Copy link

@gssbzn gssbzn commented Apr 18, 2016

  • Add .rubocop_todo.yml file and reference it on settings file
  • Apply recent rubocop autocorrections to code base

@tarcieri
Copy link
Contributor

I'd prefer to leave nothing to .rubocop_todo.yml, otherwise this seems good to me.

@gssbzn
Copy link
Author

gssbzn commented Apr 18, 2016

@tarcieri What do you mean by "leave leave nothing to .rubocop_todo.yml"
Leave the file empty? or remove any reference to it?
Some of those "todo" warnings were already on the settings file.
Regenerate the file with each fix, which IMO will be easier than having to check the rubocop.yml when they are fixed.
Let me know so I can fix the PR

@tarcieri
Copy link
Contributor

@gssbzn I mean either fix the problems or add appropriate overrides to .rubocop.yml to use in perpetuity

@gssbzn
Copy link
Author

gssbzn commented Apr 18, 2016

@tarcieri I removed the reference to the todo file
Later I can do another PR with the code style fixes needed to remove this warnings while I get more familiar with the code base

Let me know if this is fine or if there's another thing I should look at

end
rescue FFI::NotFoundError
def initialize(_opslimit, _memlimit, _digest_size = 64)
raise NotImplementedError, "scrypt not implemented in this version of libsodium"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you're declaring bankruptcy on older versions of libsodium that don't implement scrypt here?

Copy link
Author

Choose a reason for hiding this comment

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

This was the harder part to try to fix
I was checking the ffi gem, and the Sodium module and my conclusion was that the rescue was there for the sodium_function calls.
But at the same time I checked for other classes that use the sodium_function and they don't rescue in case a definition with fii fails.
Is this rescue only for scrypt case? Should the scrypt function only be exposed through RbNaCl::PasswordHash? If that's the case maybe only a require of this class in this module and handle the rescue there.

Copy link
Author

Choose a reason for hiding this comment

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

Or I can update the README to reflect the minimum libsodium

Copy link
Contributor

@tarcieri tarcieri Apr 20, 2016

Choose a reason for hiding this comment

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

Yeah, looks like scrypt was added prior to 1.0. I think it's fine to note in the README that the minimum version is 1.0.0.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@tarcieri
Copy link
Contributor

This looks fine with one noted caveat

- Apply recent rubocop autocorrections to code base
- Small code improvements to comply to rubocop
@tarcieri tarcieri merged commit 4411b66 into RubyCrypto:master Apr 20, 2016
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.

2 participants