Refactor code for maintainabilty #206

neomilium · 2020-12-31T15:01:53Z

This PR improves code structure to help owners to maintain the code and contributors to add features.

About the maintainability, this PR attempts to:

reduce code repeats (using classes to hold properties)
prevent from some name shadowing
reduce passed arguments (e.g. options were passed to many methods)
make rubocop happier ;-)

This PR also adds tests for bump, tag and changelog features.

To help contributors, this PR renames some methods with IMHO more explicit names and organizes code into classes.

Even msync have been made to manage Puppet modules, there are some users which use it for other purpose (e.g. @raphink wrote https://dev.to/camptocamp-ops/templating-puppet-control-repositories-3pk7) so I started to split specific code into PuppetModule class, while generic one is now in SourceCode class.

More details in commit logs.

neomilium · 2020-12-31T15:03:12Z

This PR have been flagged as draft due to its dependency on #202 and #204.

neomilium · 2021-04-21T10:08:17Z

This PR is so big... I started to split it to speed up review and merges.

neomilium · 2021-04-21T10:41:07Z

#212 is the first part.

@ekohl I have some unexpected spare time to contribute to modulesync this day, quicker parts are merged, more will be.

Please note that previous commit introduced some offences that will be fixed in upcomming commits (some are already available in voxpupuli#206

neomilium · 2021-10-06T11:18:45Z

This PR is stalled, waiting #219 to be merged.

codecov · 2021-10-08T19:58:30Z

Codecov Report

Merging #206 (5a03a6d) into master (5b33600) will increase coverage by 1.16%.
The diff coverage is 86.84%.

❗ Current head 5a03a6d differs from pull request most recent head 627fffa. Consider uploading reports for the commit 627fffa to get more accurate results

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   84.76%   85.93%   +1.16%     
==========================================
  Files          30       30              
  Lines         906      917      +11     
==========================================
+ Hits          768      788      +20     
+ Misses        138      129       -9

Impacted Files	Coverage Δ
lib/modulesync/git_service.rb	`70.58% <50.00%> (+16.24%)`	⬆️
lib/modulesync/source_code.rb	`97.50% <50.00%> (-2.50%)`	⬇️
lib/modulesync/git_service/factory.rb	`92.30% <80.00%> (+1.39%)`	⬆️
lib/modulesync/git_service/base.rb	`90.47% <90.90%> (+0.47%)`	⬆️
lib/modulesync/cli.rb	`100.00% <100.00%> (ø)`
lib/modulesync/git_service/gitlab.rb	`100.00% <100.00%> (ø)`
lib/modulesync/repository.rb	`93.20% <100.00%> (+0.13%)`	⬆️
spec/helpers/faker/puppet_module_remote_repo.rb	`87.20% <100.00%> (ø)`
spec/unit/modulesync/git_service_spec.rb	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b33600...627fffa. Read the comment docs.

This commit reduces the default output verbosity but allow user to increase it through `--verbose` option.

During debug, the following git error raised: fatal: ambiguous argument 'XXX': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]' With the git help recommandation the error is (a bit) more explicit: fatal: bad revision 'XXX'

neomilium · 2021-10-18T14:29:38Z

To end the story, this PR have been split in several PRs:

Its now time to append some various improvements I noted during refactoring.

smortex

LGTM

bastelfreak · 2021-10-18T16:53:05Z

lib/modulesync/cli.rb

@@ -12,14 +12,13 @@ def self.defaults
    end

    class Hook < Thor
-      class_option :project_root,


can you explain why you drop the parameter? that means we need to do a major release?

can you explain why you drop the parameter?

Yes, its not used in hook.

that means we need to do a major release?

No, I do not think so. I was just a extra parameter with no effect, AFAICS.

ekohl

Overall this looks good.

ekohl · 2021-10-19T10:12:57Z

lib/modulesync/git_service/gitlab.rb

+        endpoint += '/api/v4'
+        endpoint


Another way of writing this (but I'm not sure this is clearer)

Suggested change

endpoint += '/api/v4'

endpoint

endpoint + '/api/v4'

lib/modulesync/repository.rb

features/cli.feature

neomilium marked this pull request as draft December 31, 2020 15:01

neomilium force-pushed the code-refactoring branch 3 times, most recently from cb9243b to 51d5c96 Compare December 31, 2020 18:35

neomilium force-pushed the code-refactoring branch from 51d5c96 to 758f86a Compare April 21, 2021 10:05

neomilium added a commit to opus-codium/modulesync that referenced this pull request Apr 21, 2021

Rubocop: Update autogenerated todo file

31e9456

Please note that previous commit introduced some offences that will be fixed in upcomming commits (some are already available in voxpupuli#206

neomilium mentioned this pull request Apr 21, 2021

Refactor puppet modules properties #212

Merged

neomilium added a commit to opus-codium/modulesync that referenced this pull request Apr 21, 2021

Rubocop: Update autogenerated todo file

4438fc3

Please note that previous commit introduced some offences that will be fixed in upcomming commits (some are already available in voxpupuli#206

neomilium added a commit to opus-codium/modulesync that referenced this pull request Apr 21, 2021

Rubocop: Update autogenerated todo file

a80f0da

Please note that previous commit introduced some offences that will be fixed in upcomming commits (some are already available in voxpupuli#206

neomilium added a commit to opus-codium/modulesync that referenced this pull request Apr 21, 2021

Rubocop: Update autogenerated todo file

900b02c

Please note that previous commit introduced some offences that will be fixed in upcomming commits (some are already available in voxpupuli#206

neomilium force-pushed the code-refactoring branch from 758f86a to 020c608 Compare April 21, 2021 14:58

neomilium mentioned this pull request Apr 21, 2021

Refactor repository related code #214

Merged

neomilium force-pushed the code-refactoring branch 2 times, most recently from 2a43a87 to 36fb210 Compare April 22, 2021 14:18

neomilium mentioned this pull request Apr 22, 2021

Split generic and specific code #215

Merged

neomilium force-pushed the code-refactoring branch from 36fb210 to 5f023d3 Compare April 23, 2021 07:24

neomilium mentioned this pull request Apr 23, 2021

Are bump and changelog features relevant? #216

Open

neomilium force-pushed the code-refactoring branch 8 times, most recently from 8ae088c to 7fd4261 Compare April 27, 2021 18:06

neomilium force-pushed the code-refactoring branch from 7fd4261 to c9561eb Compare October 8, 2021 19:23

neomilium force-pushed the code-refactoring branch from c9561eb to 2492cf4 Compare October 8, 2021 19:57

neomilium force-pushed the code-refactoring branch 4 times, most recently from cac2762 to 8c8d4ad Compare October 11, 2021 17:31

neomilium added 5 commits October 18, 2021 10:15

Git: Ignore coverage/

afd636c

GitService::Factory: Factorize a #klass method

79f249b

GitService: Move specific service-related code to their class

fb79737

GitService: Allow SourceCode to handle a local repository

3501ca8

CLI: Add --verbose option

e48e620

This commit reduces the default output verbosity but allow user to increase it through `--verbose` option.

neomilium force-pushed the code-refactoring branch from 8c8d4ad to c31c64c Compare October 18, 2021 08:20

neomilium added 2 commits October 18, 2021 10:23

Aruba: Announce outputs on command failure

06a5b9a

neomilium force-pushed the code-refactoring branch from c31c64c to 5a03a6d Compare October 18, 2021 08:23

CLI: Reduce some options scope

627fffa

neomilium marked this pull request as ready for review October 18, 2021 14:29

neomilium requested review from bittner and ekohl October 18, 2021 14:30

smortex approved these changes Oct 18, 2021

View reviewed changes

neomilium mentioned this pull request Oct 18, 2021

New CLI commands #244

Merged

bastelfreak reviewed Oct 18, 2021

View reviewed changes

bastelfreak approved these changes Oct 19, 2021

View reviewed changes

ekohl reviewed Oct 19, 2021

View reviewed changes

neomilium merged commit 3c9e406 into voxpupuli:master Jan 24, 2022

neomilium deleted the code-refactoring branch January 24, 2022 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor code for maintainabilty #206

Refactor code for maintainabilty #206

neomilium commented Dec 31, 2020 •

edited

Loading

neomilium commented Dec 31, 2020

neomilium commented Apr 21, 2021

neomilium commented Apr 21, 2021

neomilium commented Oct 6, 2021

codecov bot commented Oct 8, 2021 •

edited

Loading

neomilium commented Oct 18, 2021

smortex left a comment

bastelfreak Oct 18, 2021

neomilium Oct 18, 2021 •

edited

Loading

ekohl left a comment

ekohl Oct 19, 2021

Refactor code for maintainabilty #206

Refactor code for maintainabilty #206

Conversation

neomilium commented Dec 31, 2020 • edited Loading

neomilium commented Dec 31, 2020

neomilium commented Apr 21, 2021

neomilium commented Apr 21, 2021

neomilium commented Oct 6, 2021

codecov bot commented Oct 8, 2021 • edited Loading

Codecov Report

neomilium commented Oct 18, 2021

smortex left a comment

Choose a reason for hiding this comment

bastelfreak Oct 18, 2021

Choose a reason for hiding this comment

neomilium Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

ekohl Oct 19, 2021

Choose a reason for hiding this comment

neomilium commented Dec 31, 2020 •

edited

Loading

codecov bot commented Oct 8, 2021 •

edited

Loading

neomilium Oct 18, 2021 •

edited

Loading