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

New rubocop #660

Merged
merged 14 commits into from
Jan 12, 2022
Merged

New rubocop #660

merged 14 commits into from
Jan 12, 2022

Conversation

jreidinger
Copy link
Member

draft for review of changes done in new rubocop that works with ruby 3.1

Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

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

Looks OK to me

"<a href=\"enable_secure_boot\">(#{_("enable")})</a>"
end

"#{_("Secure Boot:")} #{secure_boot ? _("enabled") : _("disabled")} #{link}"
Copy link
Member

Choose a reason for hiding this comment

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

NP: this pattern is repeated several times in the code, what about defining some helper like

def status_string(status)
  status ? _("enabled") : _("disabled")
end

# and then use in
"#{_("Secure Boot:")} #{status_string(secure_boot)} #{link}"

Copy link
Member Author

Choose a reason for hiding this comment

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

yeap, also notice it.

@@ -5,6 +5,23 @@

require "bootloader/grub2pwd"

ENCRYPTED_PASSWORD = "grub.pbkdf2.sha512.10000.774E325959D6D7BCFB7384A0245674D83D0D540A89C02FEA81E35489F8DE7ADFD93988190AD9857A0FFF363825DDF97C8F4E658D8CC49FC4A22C053B08AB3EFE.6FB19FF26FD03D85C40A33D8BA7C04E72EDE3DD5D7080C177553A4FED370F71C579AF0B15B3B93ECECEA355469A4B6D0560BFB53ED35DDA0B80F5363BFBD54E4"
Copy link
Member

Choose a reason for hiding this comment

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

Too long line?

Copy link
Member Author

Choose a reason for hiding this comment

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

strange that it does not complain. Not sure why

@@ -35,14 +35,14 @@ def initialize(bootloader: nil, secure_boot: false, trusted_boot: false, update_
end

def self.from_system
bootloader = Yast::SCR.Read(AGENT_PATH + "LOADER_TYPE")
bootloader = Yast::SCR.Read("#{AGENT_PATH}LOADER_TYPE")
Copy link
Member Author

Choose a reason for hiding this comment

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

and here is wrong auto correct. Because AGENT_PATH is not string, but Yast::Path, then result is different.

@coveralls
Copy link

coveralls commented Jan 6, 2022

Coverage Status

Coverage increased (+0.07%) to 88.232% when pulling 87944ad on new_rubocop into 5bb96b1 on master.

@jreidinger jreidinger marked this pull request as ready for review January 11, 2022 11:23
@teclator
Copy link
Contributor

It LGTM but maybe @dgdavid found something I overlooked 😜

Comment on lines +453 to +457
if status
_("enabled")
else
_("disabled")
end
Copy link
Member

Choose a reason for hiding this comment

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

NP: I'd use ternary operator here.

Copy link
Member

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

LGTM

@jreidinger jreidinger merged commit 6b8b168 into master Jan 12, 2022
@jreidinger jreidinger deleted the new_rubocop branch January 12, 2022 09:05
@yast-bot
Copy link
Contributor

✔️ Internal Jenkins job #67 successfully finished
✔️ Created IBS submit request #261908

@yast-bot
Copy link
Contributor

✔️ Public Jenkins job #104 successfully finished

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

6 participants