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: extract-functions command #870

Merged
merged 9 commits into from
Nov 17, 2019

Conversation

theethernaut
Copy link
Contributor

@theethernaut theethernaut commented Nov 5, 2019

Ready for review

Sub PR of #871

@theethernaut theethernaut force-pushed the sdk-v7-refactor-apm-extract-functions branch from 61b939c to 43c2497 Compare November 5, 2019 17:24
@theethernaut theethernaut changed the title Sdk v7 refactor apm extract functions Extracts logic from apm commands: extract-functions command Nov 5, 2019
@theethernaut theethernaut changed the base branch from develop to sdk-v7-refactor-apm November 5, 2019 17:31
@theethernaut theethernaut mentioned this pull request Nov 5, 2019
14 tasks
@theethernaut theethernaut force-pushed the sdk-v7-refactor-apm-extract-functions branch from 43c2497 to eea1680 Compare November 5, 2019 17:36
@macor161 macor161 self-requested a review November 5, 2019 17:39
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.

Nice!! Can't wait to have the entire CLI like this! 😄Only 1-2 small things. The command was successful but didn't seem to quit when I tried it: aragon acl extract-functions Test.sol

@0xGabi 0xGabi added the SDK v7 label Nov 7, 2019
@0xGabi 0xGabi requested review from dapplion and removed request for kernelwhisperer November 12, 2019 13:30
Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Looks good overall, few suggestions

@@ -89,11 +89,11 @@
"devDependencies": {
"@aragon/apps-agent": "^2.0.0",
"@aragon/apps-finance": "^3.0.0",
"@aragon/apps-payroll": "^1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid unnecessary changes in other sections of the package.json to prevent git conflicts

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that we should avoid changes unrelated to the PR. I left it there because it will probably conflict with develop if I remove it.

packages/aragon-cli/src/helpers/solidity-extractor.js Outdated Show resolved Hide resolved
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.

Great PR 💪

@0xGabi 0xGabi merged commit 109c722 into sdk-v7-refactor-apm Nov 17, 2019
@delete-merged-branch delete-merged-branch bot deleted the sdk-v7-refactor-apm-extract-functions branch November 17, 2019 08:03
macor161 pushed a commit that referenced this pull request Nov 22, 2019
* Enabling verbose in AVA tests
* Review: Extracts logic from apm commands: info command (#898)
* Extracts logic from apm commands: extract-functions command (#870)
* Extracts logic from apm commands: versions command (#891)
* Extracts logic from apm commands: packages command (#892)
* Extracts logic from apm commands: grant command (#902)
* Fix test import
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