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.

Sign up for GitHub

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

Jump to bottom

Extracts logic from apm commands: versions command #891

Merged
merged 4 commits into from
Nov 17, 2019
Merged

Extracts logic from apm commands: versions command #891

merged 4 commits into from
Nov 17, 2019

Conversation

theethernaut
Copy link
Contributor

Ready for review

Sub PR of #871

@theethernaut theethernaut mentioned this pull request Nov 8, 2019
Merged
14 tasks
@theethernaut theethernaut changed the base branch from develop to sdk-v7-refactor-apm November 9, 2019 15:07
@dapplion
Copy link
Contributor

dapplion commented Nov 9, 2019 •
edited
Loading

There are packages/aragon-cli/npm-shrinkwrap.json changes in your commit 49f42ce. Maybe it would be best to exclude them from the actual refactor work for clarity.

Also @0xGabi we might have issues with conflicting format configurations since the array in packages/aragon-cli/package.json .env.test.plugins was uncollapsed
49f42ce#diff-a9df1c6315e4fb2b010a42697a92746aR164

@0xGabi 0xGabi requested review from dapplion and removed request for kernelwhisperer November 12, 2019 13:34
@macor161
Copy link
Contributor

macor161 commented Nov 14, 2019 •
edited
Loading

There are packages/aragon-cli/npm-shrinkwrap.json changes in your commit 49f42ce. Maybe it would be best to exclude them from the actual refactor work for clarity.

@dapplion Is this comment still relevant?

@dapplion
Copy link
Contributor

There are packages/aragon-cli/npm-shrinkwrap.json changes in your commit 49f42ce. Maybe it would be best to exclude them from the actual refactor work for clarity.

@dapplion Is this comment still relevant?

No, it appears to be fixed 🎆

dapplion
dapplion suggested changes Nov 15, 2019
View reviewed changes
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.

progressHandler should be omitted in this command. Otherwise, looks good!

packages/aragon-cli/src/lib/apm/getApmRepoVersions.js Outdated Show resolved Hide resolved
0xGabi
0xGabi approved these changes Nov 17, 2019
View reviewed changes
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.

🙌

@0xGabi 0xGabi merged commit 179f103 into sdk-v7-refactor-apm Nov 17, 2019
@delete-merged-branch delete-merged-branch bot deleted the sdk-v7-refactor-apm-versions branch November 17, 2019 23:28
macor161 pushed a commit that referenced this pull request Nov 22, 2019
@theethernaut @macor161
Extracts logic from apm commands (#871)
5a3dc58 
* 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
Reviewers

@macor161 macor161 macor161 left review comments

@0xGabi 0xGabi 0xGabi approved these changes

@dapplion dapplion dapplion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@theethernaut @dapplion @macor161 @0xGabi