Refactoring - Githubissues

movitto commented 9 years ago

Refactoring of codebase so as to consolidate functionality and simplify modules. Components are now more or less independent of each other save specified integration points and are better organized.

coveralls commented 9 years ago

Coverage decreased (-1.65%) when pulling 8aef4ed7d6f167fdd3ed3f71c006967f073be26d on movitto:refactoring1 into 2a0c00ae6760cf7a9de1f3c2ff49676f3afb9b93 on ManageIQ:master.

jrafanie commented 9 years ago

@movitto Looks like the Polisher::RPM::AUTHOR constant is missing, causing the specs to fail.

This PR is enormous, which is not necessarily a bad thing, just hard to follow. From a high level, what is this PR achieving? Do you have an example before/after code snippet showing how this PR improves client code? Thanks for digging into this, refactoring is good, I'm just trying to understand it.

:cookie:

movitto commented 9 years ago

@jrafanie updated, split up the changes. Mostly structural / heirarchical, not too many changes to the external api but this makes things more modular and maintainable. Should be able to base additional changes to target functionality easier from here on out.

coveralls commented 9 years ago

Coverage increased (+0.46%) when pulling 806506b59e94091ded7f041acdcbf220154bc24c on movitto:refactoring1 into 2a0c00ae6760cf7a9de1f3c2ff49676f3afb9b93 on ManageIQ:master.

jrafanie commented 9 years ago

@movitto Can you address the following cops (where appropriate): DeprecatedClassMethods UnusedMethodArgument UselessAssignment Semicolon NonNilCheck HandleExceptions Blocks MultilineBlockChain

Some of these cops are worth looking at regardless if it's just a move.

I can't really follow what's going on here even with the split up commits. I think there just too many refactoring/reorganizing going on. Probably would be more readable if you did it in small units of change.

It seems a bit better organized so there's that. I'm fine with it, just can't say anything useful as I don't understand the design changes.

movitto commented 9 years ago

@jrafanie updated for style. Left some of the items as-is where I felt things flowed better and skipped over some of the trivial ones on the list.

Regarding the split up commits, it's probably not worth the effort atm, especially since builder only uses one callpath and that's largely unchanged. This changeset mostly serves to organize the file list which was under lib/polisher into subdirs according to generic intent and to split large classes into mixin modules (with functionality remaining the same). There are a few small changes / fixes but we can take some time to tighten things down before the next release.

coveralls commented 9 years ago

Coverage increased (+0.45%) when pulling 2faab9c0396161edc167032d9a9f69129d5bb4af on movitto:refactoring1 into 2a0c00ae6760cf7a9de1f3c2ff49676f3afb9b93 on ManageIQ:master.

miq-bot commented 9 years ago

Checked commits https://github.com/movitto/polisher-1/commit/8969080f952bb90206fbbc769c623acc3f48417f .. https://github.com/movitto/polisher-1/commit/fc29ad612f901252171633758d9e510210ebb20d with rubocop 0.27.1 89 files checked, 136 offenses detected

lib/polisher/adaptors/version_checker.rb

[ ] Style - Line 49, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for versions_for is too high. [22.16/15]
[ ] Style - Line 49, Col 5 - Metrics/CyclomaticComplexity - Cyclomatic complexity for versions_for is too high. [8/6]
[ ] Style - Line 49, Col 5 - Metrics/PerceivedComplexity - Perceived complexity for versions_for is too high. [8/7]
[ ] Style - Line 68, Col 16 - Style/EachWithObject - Use each_with_object instead of inject.

lib/polisher/gem/diff.rb

[ ] Warn - Line 30, Col 7 - Lint/HandleExceptions - Do not suppress exceptions.
[ ] Style - Line 12, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for diff is too high. [20.54/15]
[ ] Style - Line 12, Col 5 - Metrics/CyclomaticComplexity - Cyclomatic complexity for diff is too high. [7/6]
[ ] Style - Line 12, Col 5 - Metrics/PerceivedComplexity - Perceived complexity for diff is too high. [8/7]
[ ] Style - Line 22, Col 59 - Style/TrailingWhitespace - Trailing whitespace detected.
[ ] Style - Line 23, Col 33 - Style/TrailingWhitespace - Trailing whitespace detected.
[ ] Style - Line 34, Col 42 - Style/MultilineOperationIndentation - Align the operands of a condition in an unless statement spanning multiple lines.

lib/polisher/gem/files.rb

[ ] Style - Line 21, Col 68 - Style/RegexpLiteral - Use %r for regular expressions matching more than 1 '/' character.
[ ] Style - Line 58, Col 9 - Style/PredicateName - Rename has_file_satisfied_by? to file_satisfied_by?.

lib/polisher/gem/parser.rb

[ ] Style - Line 14, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for from_gemspec is too high. [23.17/15]
[ ] Style - Line 17, Col 50 - Style/MultilineOperationIndentation - Align the operands of a condition in an if statement spanning multiple lines.
[ ] Style - Line 24, Col 38 - Style/MultilineOperationIndentation - Use 2 (not 29) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 25, Col 38 - Style/MultilineOperationIndentation - Use 2 (not 29) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 27, Col 38 - Style/MultilineOperationIndentation - Use 2 (not 29) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 28, Col 38 - Style/MultilineOperationIndentation - Use 2 (not 29) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 34, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for from_json is too high. [23.09/15]

lib/polisher/gem/state.rb

[ ] Style - Line 24, Col 7 - Style/RedundantReturn - Redundant return detected.
[ ] Style - Line 66, Col 7 - Style/RedundantReturn - Redundant return detected.
[ ] Style - Line 83, Col 7 - Style/RedundantReturn - Redundant return detected.

lib/polisher/gem/versions.rb

[ ] Style - Line 24, Col 20 - Style/SymbolProc - Pass &:version as an argument to collect instead of a block.
[ ] Style - Line 54, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for dependency_versions is too high. [16.97/15]
[ ] Style - Line 76, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for versions is too high. [15.17/15]

lib/polisher/gemfile/deps.rb

[ ] Style - Line 18, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for patched is too high. [21.59/15]

lib/polisher/gemfile/parser.rb

[ ] Style - Line 17, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for parse is too high. [19.54/15]
[ ] Style - Line 35, Col 11 - Style/MultilineOperationIndentation - Use 2 (not 0) spaces for indenting an expression spanning multiple lines.

lib/polisher/git/pkg/builder.rb

[ ] Style - Line 10, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for build_srpm is too high. [23.28/15]

lib/polisher/git/pkg/repo.rb

[ ] Warn - Line 61, Col 11 - Lint/HandleExceptions - Do not suppress exceptions.
[ ] Style - Line 13, Col 7 - Style/Alias - Use alias_method instead of alias.
[ ] Style - Line 17, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for clone is too high. [23.39/15]
[ ] Style - Line 68, Col 7 - Style/Alias - Use alias_method instead of alias.

lib/polisher/git/pkg/versions.rb

[ ] Style - Line 21, Col 9 - Metrics/AbcSize - Assignment Branch Condition size for versions_for is too high. [15.43/15]

lib/polisher/mixins/vendored_deps.rb

[ ] Style - Line 18, Col 27 - Style/EachWithObject - Use each_with_object instead of inject.

lib/polisher/rpm.rb

[ ] Warn - Line 15, Col 5 - Lint/HandleExceptions - Do not suppress exceptions.

lib/polisher/rpm/requirement/comparison.rb

[ ] Style - Line 11, Col 9 - Style/MultilineOperationIndentation - Use 2 (not 0) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 12, Col 9 - Style/MultilineOperationIndentation - Use 2 (not 0) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 13, Col 9 - Style/MultilineOperationIndentation - Use 2 (not 0) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 22, Col 17 - Style/MultilineOperationIndentation - Use 2 (not 8) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 23, Col 17 - Style/MultilineOperationIndentation - Use 2 (not 8) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 27, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for min_satisfying_version is too high. [16.76/15]
[ ] Style - Line 41, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for max_satisfying_version is too high. [15.23/15]
[ ] Style - Line 41, Col 7 - Metrics/CyclomaticComplexity - Cyclomatic complexity for max_satisfying_version is too high. [7/6]
[ ] Style - Line 53, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for min_failing_version is too high. [15.52/15]
[ ] Style - Line 83, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for matches? is too high. [25.71/15]
[ ] Style - Line 83, Col 7 - Metrics/CyclomaticComplexity - Cyclomatic complexity for matches? is too high. [7/6]
[ ] Style - Line 93, Col 25 - Style/MultilineOperationIndentation - Use 2 (not 16) spaces for indenting an expression spanning multiple lines.

lib/polisher/rpm/requirement/gem_reference.rb

[ ] Style - Line 18, Col 9 - Metrics/AbcSize - Assignment Branch Condition size for from_gem_dep is too high. [16.03/15]
[ ] Style - Line 35, Col 9 - Style/DoubleNegation - Avoid the use of double negation (!!).
[ ] Style - Line 42, Col 9 - Style/DoubleNegation - Avoid the use of double negation (!!).
[ ] Style - Line 42, Col 54 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.

lib/polisher/rpm/requirement/parser.rb

[ ] Style - Line 15, Col 9 - Metrics/AbcSize - Assignment Branch Condition size for parse is too high. [16.31/15]

lib/polisher/rpm/spec/check.rb

[ ] Style - Line 9, Col 11 - Style/PredicateName - Rename has_check? to check?.
[ ] Style - Line 10, Col 9 - Style/DoubleNegation - Avoid the use of double negation (!!).

lib/polisher/rpm/spec/comparison.rb

[ ] Style - Line 18, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for compare is too high. [58.09/15]
[ ] Style - Line 18, Col 7 - Metrics/CyclomaticComplexity - Cyclomatic complexity for compare is too high. [11/6]
[ ] Style - Line 18, Col 7 - Metrics/MethodLength - Method has too many lines. [33/25]
[ ] Style - Line 18, Col 7 - Metrics/PerceivedComplexity - Perceived complexity for compare is too high. [13/7]
[ ] Style - Line 24, Col 16 - Style/SymbolProc - Pass &:specifier as an argument to collect instead of a block.
[ ] Style - Line 35, Col 23 - Style/DeprecatedHashMethods - Hash#has_key? is deprecated in favor of Hash#key?.
[ ] Style - Line 54, Col 23 - Style/DeprecatedHashMethods - Hash#has_key? is deprecated in favor of Hash#key?.

lib/polisher/rpm/spec/gem_requirements.rb

[ ] Style - Line 24, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for has_all_requirements_for? is too high. [18.11/15]
[ ] Style - Line 24, Col 11 - Style/PredicateName - Rename has_all_requirements_for? to all_requirements_for?.
[ ] Style - Line 42, Col 9 - Style/SymbolProc - Pass &:gem? as an argument to select instead of a block.
[ ] Style - Line 47, Col 9 - Style/SymbolProc - Pass &:gem? as an argument to select instead of a block.

lib/polisher/rpm/spec/parser.rb

[ ] Style - Line 29, Col 9 - Metrics/AbcSize - Assignment Branch Condition size for parse is too high. [73.46/15]
[ ] Style - Line 29, Col 9 - Metrics/CyclomaticComplexity - Cyclomatic complexity for parse is too high. [23/6]
[ ] Style - Line 29, Col 9 - Metrics/MethodLength - Method has too many lines. [52/25]
[ ] Style - Line 29, Col 9 - Metrics/PerceivedComplexity - Perceived complexity for parse is too high. [25/7]
[ ] Style - Line 40, Col 33 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 41, Col 33 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 44, Col 19 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 45, Col 33 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 48, Col 32 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 51, Col 32 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 54, Col 29 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 58, Col 57 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 61, Col 63 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 71, Col 29 - Style/PerlBackrefs - Avoid the use of Perl-style backrefs.
[ ] Style - Line 99, Col 11 - Style/SymbolProc - Pass &:strip as an argument to collect! instead of a block.

lib/polisher/rpm/spec/subpackages.rb

[ ] Style - Line 18, Col 11 - Style/PredicateName - Rename has_doc_subpkg? to doc_subpkg?.

lib/polisher/rpm/spec/updater.rb

[ ] Style - Line 25, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for update_deps_from is too high. [26/15]
[ ] Style - Line 30, Col 21 - Style/MultilineOperationIndentation - Use 2 (not 10) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 36, Col 21 - Style/MultilineOperationIndentation - Use 2 (not 10) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 40, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for update_files_from is too high. [44.55/15]
[ ] Style - Line 40, Col 7 - Metrics/CyclomaticComplexity - Cyclomatic complexity for update_files_from is too high. [7/6]
[ ] Style - Line 40, Col 7 - Metrics/MethodLength - Method has too many lines. [27/25]
[ ] Style - Line 40, Col 7 - Metrics/PerceivedComplexity - Perceived complexity for update_files_from is too high. [8/7]
[ ] Style - Line 78, Col 42 - Style/SymbolProc - Pass &:rpmize as an argument to collect instead of a block.
[ ] Style - Line 93, Col 23 - Style/StringLiteralsInInterpolation - Prefer single-quoted strings inside interpolations.
[ ] Style - Line 122, Col 32 - Style/MultilineOperationIndentation - Use 2 (not 23) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 131, Col 35 - Style/MultilineOperationIndentation - Use 2 (not 26) spaces for indenting an expression spanning multiple lines.

lib/polisher/targets/bodhi.rb

[ ] Style - Line 15, Col 24 - Style/MultilineOperationIndentation - Align the operands of an expression in an assignment spanning multiple lines.
[ ] Style - Line 16, Col 24 - Style/MultilineOperationIndentation - Align the operands of an expression in an assignment spanning multiple lines.

lib/polisher/targets/fedora.rb

[ ] Style - Line 17, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for gems_owned_by is too high. [17.46/15]

lib/polisher/targets/koji/builder.rb

[ ] Style - Line 14, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for build is too high. [15.52/15]

lib/polisher/targets/koji/diff.rb

[ ] Style - Line 14, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for diff is too high. [19.34/15]

lib/polisher/targets/koji/versions.rb

[ ] Style - Line 15, Col 11 - Style/PredicateName - Rename has_build? to build?.
[ ] Style - Line 22, Col 11 - Style/PredicateName - Rename has_build_satisfying? to build_satisfying?.

lib/polisher/targets/yum.rb

[ ] Style - Line 19, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for version_for is too high. [16.88/15]

lib/polisher/util/conf_helpers.rb

[ ] Style - Line 24, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for conf_attr is too high. [16.55/15]

lib/polisher/util/core_ext.rb

[ ] Style - Line 39, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for rpmize is too high. [19.52/15]

spec/git/pkg_spec.rb

[ ] Style - Line 134, Col 11 - Style/Lambda - Use the new lambda literal syntax ->(params) {...}.
[ ] Style - Line 134, Col 17 - Style/SpaceBeforeBlockBraces - Space missing to the left of {.
[ ] Style - Line 205, Col 21 - Style/MultilineOperationIndentation - Use 2 (not 12) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 215, Col 21 - Style/MultilineOperationIndentation - Use 2 (not 12) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 232, Col 21 - Style/MultilineOperationIndentation - Use 2 (not 12) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 233, Col 21 - Style/MultilineOperationIndentation - Use 2 (not 12) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 249, Col 23 - Style/MultilineOperationIndentation - Use 2 (not 12) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 277, Col 24 - Style/MultilineOperationIndentation - Use 2 (not 15) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 278, Col 24 - Style/MultilineOperationIndentation - Use 2 (not 15) spaces for indenting an expression spanning multiple lines.

spec/git/repo_spec.rb

[ ] Warn - Line 68, Col 28 - Lint/Void - Operator == used in void context.
[ ] Warn - Line 71, Col 26 - Lint/Void - Operator == used in void context.
[ ] Style - Line 20, Col 27 - Style/MultilineOperationIndentation - Use 2 (not 18) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 21, Col 27 - Style/MultilineOperationIndentation - Use 2 (not 18) spaces for indenting an expression spanning multiple lines.

spec/mixins/versioned_dependencies_spec.rb

[ ] Warn - Line 23, Col 44 - Lint/Void - Operator == used in void context.

spec/targets/fedora_spec.rb

[ ] Style - Line 22, Col 24 - Style/MultilineOperationIndentation - Use 2 (not 15) spaces for indenting an expression spanning multiple lines.

spec/targets/koji_spec.rb

[ ] Warn - Line 69, Col 57 - Lint/Void - Operator == used in void context.
[ ] Warn - Line 70, Col 57 - Lint/Void - Operator == used in void context.
[ ] Warn - Line 74, Col 56 - Lint/Void - Operator == used in void context.
[ ] Warn - Line 77, Col 54 - Lint/Void - Operator == used in void context.
[ ] Warn - Line 81, Col 58 - Lint/Void - Operator == used in void context.
[ ] Warn - Line 82, Col 58 - Lint/Void - Operator == used in void context.
[ ] Style - Line 59, Col 54 - Style/WordArray - Use %w or %W for array of words.
[ ] Style - Line 104, Col 63 - Style/WordArray - Use %w or %W for array of words.
[ ] Style - Line 117, Col 63 - Style/WordArray - Use %w or %W for array of words.
[ ] Style - Line 155, Col 24 - Style/MultilineOperationIndentation - Use 2 (not 15) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 161, Col 24 - Style/MultilineOperationIndentation - Use 2 (not 15) spaces for indenting an expression spanning multiple lines.
[ ] Style - Line 182, Col 11 - Style/Lambda - Use the new lambda literal syntax ->(params) {...}.

spec/targets/yum_spec.rb

[ ] Style - Line 16, Col 24 - Style/MultilineOperationIndentation - Use 2 (not 15) spaces for indenting an expression spanning multiple lines.

spec/util/component_spec.rb

[ ] Style - Line 9, Col 20 - Style/ClassAndModuleChildren - Use nested module/class definitions instead of compact style.

spec/util/core_ext_spec.rb

[ ] Warn - Line 53, Col 31 - Lint/Void - Operator == used in void context.
[ ] Warn - Line 64, Col 28 - Lint/Void - Operator == used in void context.
[ ] Warn - Line 65, Col 27 - Lint/Void - Operator == used in void context.

coveralls commented 9 years ago

Coverage increased (+0.45%) when pulling fc29ad612f901252171633758d9e510210ebb20d on movitto:refactoring1 into 2a0c00ae6760cf7a9de1f3c2ff49676f3afb9b93 on ManageIQ:master.

movitto commented 9 years ago

@jrafanie any objections to merging this? Since most changes are to internal layout / style, I'd suggest we get this in / iron additional things out via the spec suite (see followup pr's). The overall codebase should be fairly stable after this point as most things are now componentized / compartmentalized.

jrafanie commented 9 years ago

Sure @movitto, I haven't been spending too much time in this area, so feel free to do multi-step restructuring, we can bump the version to 0.11.0 when you're done to shake out any thing that breaks.

My only suggestion is to keep your eye on the rubocop warnings and code climate/coveralls scores... they're good general guidelines... Let's drop complexity where we can to make small objects that do one thing well.

ManageIQ / polisher

Refactoring #125