Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Extend the Digest::UUID module with the clean method #81

Merged
merged 4 commits into from
Jul 9, 2020
Merged

Extend the Digest::UUID module with the clean method #81

merged 4 commits into from
Jul 9, 2020

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Jun 10, 2020

Since Rails 5.1+ gives us the Digest::UUID class, let's extend it with the clean method here. The eventual goal is to update the core Hardware class with this method, and remove miq-uuid from gems-pending.

@djberg96
Copy link
Contributor Author

We can ignore the Hakiri warning, it's a bug on their end.

@bdunne
Copy link
Member

bdunne commented Jun 10, 2020

I know you're just moving code from a different repo, but when I put on my non-ManageIQ more_core_extensions hat 🎩 I wonder if this should instead be one or more of:

  • String#to_uuid
  • String#format_uuid
  • Digest::UUID#from_string(string)
  • Digest::UUID#format(string)

@Fryguy Any thoughts?

@djberg96 djberg96 changed the title Extend the Digest::UUID method with clean_guid Extend the Digest::UUID module with the clean method Jun 10, 2020
@Fryguy
Copy link
Member

Fryguy commented Jun 10, 2020

I don't like the string patches, because to_uuid I would expect some kind of UUID object out the other side, not for it to be cleaned. Same with .from_string. .format is not bad, but I think .clean is clearer.

@miq-bot
Copy link
Member

miq-bot commented Jun 11, 2020

Checked commits https://github.com/djberg96/more_core_extensions/compare/3d731e860bbc40a42868f5e65f743b62515b06b3~...d252b90d5dd2e491616215de754a3f9e45eac8f0 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
4 files checked, 2 offenses detected

lib/more_core_extensions/core_ext/digest/uuid.rb

@djberg96 djberg96 closed this Jun 11, 2020
@djberg96 djberg96 reopened this Jun 11, 2020
@djberg96
Copy link
Contributor Author

@Fryguy Look ok now?

@bdunne
Copy link
Member

bdunne commented Jul 9, 2020

@Fryguy Any concerns?

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants