General refactoring. #56

woodruffw · 2016-08-01T00:59:32Z

Another low-priority PR, just cleans up the code a bit and adds a .rubocop.yml for future style enforcement.

rubocop complains about roughly 100 other things in the lib/ tree, although most of them are even less significant (methods over 10 lines, for example).
All of the unresolved violations are here: ruby-macho-rubocop.txt

cc @UniqMartin

UniqMartin · 2016-08-05T13:36:16Z

.rubocop.yml

@@ -0,0 +1,6 @@
+StringLiterals:
+  Enabled: false


You're missing the Style/ prefix in the preceding line and instead of simply disabling the check, it's better to reconfigure it to our preferred style:

Style/StringLiterals: EnforcedStyle: double_quotes

You also most likely want to add the following for consistency with the above and with Homebrew code in general:

Style/StringLiteralsInInterpolation: EnforcedStyle: double_quotes

The second part will also take care of some of the complaints that remained in the RuboCop output.

woodruffw · 2016-08-05T13:56:40Z

Sorry for the rookie mistakes regarding RuboCop - this is my first time configuring/using it 😅.

UniqMartin · 2016-08-05T14:00:02Z

Sorry for the rookie mistakes regarding RuboCop - this is my first time configuring/using it

No worries, I'm happy to point you into the right direction (or what I believe to be the right direction). I certainly had more experience tweaking those rules in the context of Homebrew. Overall I'm very happy that you came up with this PR on your own. 👍

UniqMartin · 2016-08-05T14:12:09Z

rubocop complains about roughly 100 other things in the lib/ tree, although most of them are even less significant (methods over 10 lines, for example). All of the unresolved violations are here: ruby-macho-rubocop.txt

Some of them will already vanish after addressing my comments. Others, like the length and complexity metrics are something I'm less convinced of (or at least I find the limits to be quite harsh), thus I'd be personally alright with:

Metrics/AbcSize:
  Enabled: false
Metrics/ClassLength:
  Enabled: false
Metrics/CyclomaticComplexity:
  Enabled: false
Metrics/MethodLength:
  Enabled: false
Metrics/ModuleLength:
  Enabled: false
Metrics/ParameterLists:
  Enabled: false
Metrics/PerceivedComplexity:
  Enabled: false

We can sort out most of the remaining items either in the next iteration of this PR or just ignore them for the time being and deal with them in a later PR.

Pro Tip: Use rubocop -DES for extra details like the cop name responsible for the output and links to the corresponding section of the style guide. I also like to append -f progress -f offenses to the argument list to get an additional summary of the various types of offenses that were raised.

woodruffw · 2016-08-05T14:16:55Z

Metrics/AbcSize:
Enabled: false
Metrics/ClassLength:
Enabled: false
Metrics/CyclomaticComplexity:
Enabled: false
Metrics/MethodLength:
Enabled: false
Metrics/ModuleLength:
Enabled: false
Metrics/ParameterLists:
Enabled: false
Metrics/PerceivedComplexity:
Enabled: false

This looks good to me! I'm going to make all the changes you've recommended tonight.

Thanks for the tip about the additional RuboCop flags 😄

woodruffw · 2016-08-05T23:31:30Z

RuboCop still complains about unaligned parameters/arrays/cases, so I'm probably going to add those to the .rubocop.yml unless you think we should address them here.

There's also the matter of the unfortunately named MachOFile#set_ncmds and MachOFile#set_sizeofcmds. How do update_ncmds and update_sizeofcmds sound to you? Alternatively, we could do something like raw_ncmds=...

UniqMartin · 2016-08-06T10:33:38Z

RuboCop still complains about unaligned parameters/arrays/cases, so I'm probably going to add those to the .rubocop.yml unless you think we should address them here.

I just went ahead and pushed a few commits that address these, partially to see for myself how this will change the code. I'd say that we should address them (I only focused my attention on the lib/ directory; it is clean except for one documentation complaint). Feel free to rearrange, drop, or squash any of my commits that I so boldly pushed to your branch.

There's also the matter of the unfortunately named MachOFile#set_ncmds and MachOFile#set_sizeofcmds. How do update_ncmds and update_sizeofcmds sound to you?

I'm in favor of update_*. That's already the verb we use in the documentation comment.

UniqMartin · 2016-08-06T10:35:01Z

lib/macho/macho_file.rb

+                        MachO::Section64
+                      else
+                        raise ArgumentError, "not a valid segment"
+                      end


If this looks unappealing to you, we could steal the following snippet from Homebrew's .rubocop.yml to gain a bit more flexibility with these:

# we prefer compact if-else-end/case-when-end alignment Lint/EndAlignment: AlignWith: variable Style/CaseIndentation: IndentWhenRelativeTo: end

Yeah, I do generally prefer the original form.
I'll add those instructions to the .rubocop.yml.

👍

I can see arguments for both styles, but I'm happy with either (but more used to the unindented style due to my Homebrew work).

woodruffw · 2016-08-06T14:44:21Z

Looks great to me! I'll squash these down into the RuboCop and general refactoring commits in a moment.

I only focused my attention on the lib/ directory; it is clean except for one documentation complaint)

As did I. It might make sense at some point in the future to align the unit tests with our RuboCop enforced style, but that's not particularly high on my list of priorities for ruby-macho 😄

woodruffw · 2016-08-06T15:01:45Z

lib/macho/macho_file.rb

      segments.each do |seg|
        sections(seg).each do |sect|
-          next if sect.size == 0
+          next if sect.size == 0 # rubocop:disable Style/ZeroLengthPredicate


Looks like RuboCop still complains here about a Style/NumericPredicate violation. I'd personally just prefer zero? here, since it's equally readable (IMO, of course) and we don't have to disable two checks in-line.

(Ignore all of that, replacing this with Section#empty?)

👍 To quote from the other related code comment:

I was still on RuboCop 0.41.2. That's why I didn't notice this was now causing multiple complaints and this suggestion isn't exactly practical.

UniqMartin · 2016-08-06T22:39:54Z

I only focused my attention on the lib/ directory; it is clean except for one documentation complaint)

As did I. It might make sense at some point in the future to align the unit tests with our RuboCop enforced style, but that's not particularly high on my list of priorities for ruby-macho 😄

Great! I think it's sufficient to tweak those style issues incrementally as the code gets modified (or maybe in a follow-up PR for the test suite as a whole). A good way to improve incrementally is to let your editor highlight RuboCop style violations for you while coding. I'm using SublimeLinter with its RuboCop plugin for that and loving it.

UniqMartin · 2016-08-06T22:44:36Z

.rubocop.yml

+Style/TrailingCommaInArguments:
+  EnforcedStyleForMultiline: no_comma
+
+# TODO: disable in the future


Should that be “re-enable in the future”?

Yep. I was thinking disable as in "disable these lines", but re-enable makes much more sense.

woodruffw · 2016-08-06T22:46:20Z

A good way to improve incrementally is to let your editor highlight RuboCop style violations for you while coding. I'm using SublimeLinter with its RuboCop plugin for that and loving it.

Thanks! I knew about SublimeLinter, but I didn't know about its RuboCop plugin. I'll install it now 😄

UniqMartin · 2016-08-06T22:49:33Z

lib/macho/load_commands.rb

          raw_string = view.raw_data.slice(lc_str_abs..lc_end)
-          @string, null_byte, _ = raw_string.partition("\x00")
-          raise LCStrMalformedError.new(lc) if null_byte.empty?
+          @string, null_byte, = raw_string.partition("\x00")


I'm not liking this resolution. The trailing comma on the left-hand side is too easy to overlook. How about the following?

@string, null_byte, _padding = raw_string.partition("\x00")

I was thinking the same thing. _padding sounds good to me!

woodruffw · 2016-08-07T15:33:50Z

Rebased; I'm happy with the state of this PR if you are 🎉

UniqMartin · 2016-08-07T15:40:30Z

Rebased … and checked whether rubocop complains about any of the new/changed code? 😉

woodruffw · 2016-08-07T15:43:20Z

Hmm, RuboCop doesn't like that we never use the options arguments to the modification methods inMachOFile. I guess I'll make them _options if (until?) we end up needing them.

UniqMartin · 2016-08-07T16:43:49Z

Could you try and adopt a workflow that looks something like this?

Make a change.
Self-review and basic sanity check (in this case re-running rubocop).
Push change.

Sorry if I'm wronging you with this, but it feels like you're sometimes skipping the second step. I don't think I should be pointing out stuff like in the following terminal output and we could be both more productive if I didn't have to, not only because of the incurred roundtrip delay.

$ rubocop -D -f simple lib
== lib/macho/load_commands.rb ==
C:403:  1: Style/EmptyLines: Extra blank line detected.
== lib/macho/open.rb ==
C:  1:  1: Style/Documentation: Missing top-level module documentation comment.

12 files inspected, 2 offenses detected

(And if in doubt: No, I'm not referring to the second offense that has been there from the start. I'm talking about the other one that has been introduced due to the rebase.)

woodruffw · 2016-08-07T16:48:01Z

Sorry if I'm wronging you with this

You're not, I haven't been proactive about checking work that I think is correct. Sorry for imposing the extra burden on you, it's not fair of me to do.

(That last offense looks like another one of RuboCop's screw-ups - the MachO module is documented in macho.rb.)

Bring code in line with RuboCop. Freeze constant objects, raise exceptions without #new where possible. Eliminate unnecessary `self` prefixes and correct formatting. Use 1.9-style hashes where possible. Align parameters and array items. Rename 'set_*' methods to 'update_*'. Rename 'get_*' methods to 'populate_*'

UniqMartin · 2016-08-07T17:08:09Z

Thanks! 🙇

(That last offense looks like another one of RuboCop's screw-ups - the MachO module is documented in macho.rb.)

We can look into this later. I'm not 100% sure how RuboCop is performing the check, but I'm relatively confident it works on a file by file basis and thus cannot correlate information from different files. (It's perfectly normal to let it check one file only.) Since modules and classes can be reopened any time, I suspect it's using some heuristics to determine whether the module is new (and thus needs to be documented) or is just being reopened and added to …

woodruffw · 2016-08-07T17:10:35Z

Yeah, that would make sense. For the time being I'm happy with just ignoring that line in rubocop's output (disabling it probably wouldn't be good, since we don't want to leave future modules/classes accidentally undocumented).

UniqMartin · 2016-08-07T17:23:02Z

Yes, agreed. It's not exactly bothering me and we can ignore it for the time being.

Slightly off topic, but I guess you'd like to cut a new release soon? Even if it's not 1.0 time yet, I think we have accumulated quite a few important changes since the last release and it would make sense to cut a new one. I think it would also makes sense to bring this new release to Homebrew proper, so that the test bot can throw some real world binaries at it and we can see how this goes, not to mention that we'll need to make some minor adjustments to the code that is bridging between the Homebrew-internal API and the ruby-macho API.

woodruffw · 2016-08-07T17:24:49Z

I was just about to! It'll be up in a moment.

woodruffw · 2016-08-07T17:28:24Z

I'll also open a PR on brew to bring the updated ruby-macho in and update our interfacing code.

woodruffw added the in progress label Aug 1, 2016

woodruffw added this to the Release 1.0.0 milestone Aug 1, 2016

woodruffw mentioned this pull request Aug 1, 2016

1.0 release #48

Closed

9 tasks

UniqMartin reviewed Aug 5, 2016
View reviewed changes

woodruffw force-pushed the general-refactoring branch from 39b8e03 to 2eb518a Compare August 5, 2016 23:28

UniqMartin reviewed Aug 6, 2016
View reviewed changes

woodruffw reviewed Aug 6, 2016
View reviewed changes

woodruffw force-pushed the general-refactoring branch from 6f753fc to d169801 Compare August 6, 2016 15:09

UniqMartin reviewed Aug 6, 2016
View reviewed changes

woodruffw force-pushed the general-refactoring branch from d169801 to a4eecf8 Compare August 6, 2016 22:48

UniqMartin reviewed Aug 6, 2016
View reviewed changes

woodruffw force-pushed the general-refactoring branch from a4eecf8 to 5ac8025 Compare August 6, 2016 22:51

UniqMartin removed the in progress label Aug 7, 2016

woodruffw force-pushed the general-refactoring branch 2 times, most recently from 72613bc to 6891bcd Compare August 7, 2016 15:28

Style: Add .rubocop.yml for style checking.

1ca459c

woodruffw force-pushed the general-refactoring branch from 6891bcd to e0adad9 Compare August 7, 2016 15:31

woodruffw force-pushed the general-refactoring branch from e0adad9 to e089598 Compare August 7, 2016 15:49

woodruffw force-pushed the general-refactoring branch from e089598 to cba3369 Compare August 7, 2016 16:48

UniqMartin merged commit 3eb6ac3 into master Aug 7, 2016

UniqMartin deleted the general-refactoring branch August 7, 2016 17:03

lock bot added the outdated label Jan 1, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020

Uh oh!

General refactoring. #56

General refactoring. #56

Uh oh!

Conversation

woodruffw commented Aug 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woodruffw commented Aug 5, 2016

Uh oh!

UniqMartin commented Aug 5, 2016

Uh oh!

UniqMartin commented Aug 5, 2016

Uh oh!

woodruffw commented Aug 5, 2016

Uh oh!

woodruffw commented Aug 5, 2016

Uh oh!

UniqMartin commented Aug 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woodruffw commented Aug 6, 2016

Uh oh!

woodruffw Aug 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UniqMartin commented Aug 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woodruffw commented Aug 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woodruffw commented Aug 7, 2016

Uh oh!

UniqMartin commented Aug 7, 2016

Uh oh!

woodruffw commented Aug 7, 2016

Uh oh!

UniqMartin commented Aug 7, 2016

Uh oh!

woodruffw commented Aug 7, 2016

Uh oh!

UniqMartin commented Aug 7, 2016

Uh oh!

woodruffw commented Aug 7, 2016

Uh oh!

UniqMartin commented Aug 7, 2016

Uh oh!

woodruffw commented Aug 7, 2016

Uh oh!

woodruffw commented Aug 7, 2016

Uh oh!

Uh oh!

woodruffw Aug 6, 2016 •

edited

Loading