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

Validator tidy #147

Merged
merged 5 commits into from
Oct 27, 2014
Merged

Validator tidy #147

merged 5 commits into from
Oct 27, 2014

Conversation

iainbeeston
Copy link
Contributor

I was looking through some of the metrics on code climate and decided to tidy up some duplicate code. Here's a summary:

  1. Right now, when we check for the presence of optional gems (yajl, uuidtools etc) we have a big ugly if statement whose condition is a begin... rescue block (for backwards compatibility reasons). I've updated it to use a more up-to-date api which doesn't need the block (should work on ruby 1.9+)
  2. The code for generating a uri (or file uri) is duplicated in three places in Validator. I've refactored that out to a new normalized_uri method
  3. I've renamed fake_uri to fake_uuid as it returns a uuid (created from a uri) - it does not return a uri

@pd
Copy link
Contributor

pd commented Oct 27, 2014

👍

I've been curious if we really need the UUID code at all, or if we could just replace it with Digest::SHA1.hexdigest or such. It'd eliminate the uuid.rb file (which gets an F in code climate =), and is even a little bit faster. But it'd be backwards-incompatible for anyone expecting exact error messages, since the UUID is currently referenced in validation errors, so maybe it's not worth it.

@hoxworth
Copy link
Contributor

I'd like to keep the UUID generation at least until the next major version upgrade (which we can discuss a roadmap for), just to maintain backwards compatibility. I'll open a 3.0 milestone and start creating tickets in it, including this.

👍 on the changes

hoxworth added a commit that referenced this pull request Oct 27, 2014
@hoxworth hoxworth merged commit a5a6395 into voxpupuli:master Oct 27, 2014
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.

3 participants