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

Integrate miq-unicode.rb into smartstate #149

Merged
merged 5 commits into from
Jan 21, 2021
Merged

Integrate miq-unicode.rb into smartstate #149

merged 5 commits into from
Jan 21, 2021

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Jan 20, 2021

This moves the methods from miq-unicode.rb from gems-pending into the smartstate repo directly. Since we're reopening a base class (String), I thought this would be a good opportunity to use Ruby's refinements. This ensures that we don't cause any entanglements with any other libraries, now or in the future.

Part of the gems-pending effort: ManageIQ/manageiq-gems-pending#231

Note that one other repo (virtfs-ntfs) uses a few of these methods so that will have to be addressed before we can remove miq-unicode.rb from gems-pending. Nevermind, that repo was archived.

The virt_disk repo uses identical methods but defines them locally.

@Fryguy
Copy link
Member

Fryguy commented Jan 20, 2021

Note that a lot of this code does not have automated tests. Can you verify at least one of these calls is being executed through automated tests so we can know that everything works properly?

@Fryguy
Copy link
Member

Fryguy commented Jan 20, 2021

cc @agrare

@djberg96
Copy link
Contributor Author

djberg96 commented Jan 20, 2021

Note that a lot of this code does not have automated tests.

Like, none of it as far as I can tell. Oof.

Can you verify at least one of these calls is being executed through automated tests so we can know that everything works properly?

I'll try to whip some specs for one of the files at least.

Update: ok, I found one test so far that calls it internally and it still works:

https://github.com/ManageIQ/manageiq-smartstate/blob/master/spec/metadata/VmConfig/VmConfig_spec.rb

lib/miq_unicode.rb Outdated Show resolved Hide resolved
lib/miq_unicode.rb Outdated Show resolved Hide resolved
lib/miq_unicode.rb Outdated Show resolved Hide resolved
lib/miq_unicode.rb Outdated Show resolved Hide resolved
lib/miq_unicode.rb Outdated Show resolved Hide resolved
@djberg96
Copy link
Contributor Author

I copied over the specs from gems-pending as well (and updated them slightly). They pass.

@miq-bot
Copy link
Member

miq-bot commented Jan 20, 2021

Checked commits https://github.com/djberg96/manageiq-smartstate/compare/f9387b774fb3d42af016249e3629ea8553af0ca6~...f15a52df282def86f9e72918b5f5d2116c40904e with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
18 files checked, 12 offenses detected

lib/fs/fat32/directory_entry.rb

lib/miq_unicode.rb

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM. Will leave it to @chessbyte or @agrare

@chessbyte chessbyte merged commit 9333dfa into ManageIQ:master Jan 21, 2021
Fryguy added a commit that referenced this pull request Jan 28, 2021
Fixed:
- Fixed MiqWin32::Software.DecodeProductKey #145

Removed:
- Remove references to miq-unicode from manageiq-gems-pending #149
- Remove references to runcmd from manageiq-gems-pending #153
- Remove Metakit related code which never worked #150
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants