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

Extracts logic from apm commands #871

Merged
merged 9 commits into from
Nov 22, 2019
Merged

Extracts logic from apm commands #871

merged 9 commits into from
Nov 22, 2019

Conversation

theethernaut
Copy link
Contributor

@theethernaut theethernaut commented Nov 5, 2019

WIP

Commands:

Misc:

  • Tests have the same default values comment, they should all be /* Default values */
  • Write tests for ACL.js
  • Write tests for solidity-extractor.js
  • Packages command does not need to import APM
  • Most of the apm refactors are similar, especially in how they begin, can something be abstracted from them?
  • Unify conversions from ens-registry to ensRegistryAddress in scripts and tests, or remove the need for the conversion altogether ) -> will be done on removing deprecated code PR
  • When removing tasks, lost the little green tick on console output titles... need a way to reproduce it on all the commands that I changed

Note: discussion about future steps moved #947

@0xGabi 0xGabi changed the title Extracts logic from apm commands WIP Extracts logic from apm commands Nov 17, 2019
@dapplion
Copy link
Contributor

@0xGabi Let's review this one as a whole once all other sub-PRs are merged

@codecov
Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #871 into develop will increase coverage by 4.38%.
The diff coverage is 51.16%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #871      +/-   ##
===========================================
+ Coverage    10.85%   15.24%   +4.38%     
===========================================
  Files           93       98       +5     
  Lines         2404     2486      +82     
===========================================
+ Hits           261      379     +118     
+ Misses        2143     2107      -36
Impacted Files Coverage Δ
packages/aragon-cli/src/commands/apm_cmds/grant.js 0% <0%> (ø) ⬆️
packages/aragon-cli/src/commands/apm_cmds/info.js 0% <0%> (ø) ⬆️
...kages/aragon-cli/src/commands/apm_cmds/versions.js 0% <0%> (ø) ⬆️
...kages/aragon-cli/src/commands/extract-functions.js 0% <0%> (ø)
packages/aragon-cli/src/lib/apm/util/acl.js 0% <0%> (ø)
...kages/aragon-cli/src/commands/apm_cmds/packages.js 0% <0%> (ø) ⬆️
...gon-cli/src/commands/dao_cmds/utils/execHandler.js 0% <0%> (ø) ⬆️
...li/src/commands/apm_cmds/util/generate-artifact.js 0% <0%> (ø) ⬆️
...s/aragon-cli/src/lib/apm/getApmRegistryPackages.js 100% <100%> (ø)
...kages/aragon-cli/src/lib/apm/getApmRepoVersions.js 100% <100%> (ø)
... and 11 more

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 0574c8b...dfc3d7a. Read the comment docs.

@0xGabi 0xGabi changed the title WIP Extracts logic from apm commands Extracts logic from apm commands Nov 21, 2019
Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

LGTM 🙌

I think we can merge this to keep progress for the next iteration. Tackling the misc points.

@0xGabi 0xGabi mentioned this pull request Nov 21, 2019
@dapplion
Copy link
Contributor

Agree, will review this PR as a whole today

@0xGabi 0xGabi mentioned this pull request Nov 21, 2019
43 tasks
@macor161 macor161 self-requested a review November 22, 2019 02:09
Copy link
Contributor

@macor161 macor161 left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@macor161 macor161 merged commit 5a3dc58 into develop Nov 22, 2019
@delete-merged-branch delete-merged-branch bot deleted the sdk-v7-refactor-apm branch November 22, 2019 02:13
@dapplion
Copy link
Contributor

Although it's already merged, looks good to me too. It would need a second iteration to make things consistent and test it as a whole.

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

4 participants