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

Extract logic from "dao apps" and "dao new" #955

Merged
merged 41 commits into from
Dec 4, 2019
Merged

Extract logic from "dao apps" and "dao new" #955

merged 41 commits into from
Dec 4, 2019

Conversation

macor161
Copy link
Contributor

@macor161 macor161 commented Nov 25, 2019

🦅 Pull Request

Extract logic from dao apps and dao new to the lib folder.
Supersedes #944

Part of the integration tests are dependent on IPFS. They will therefore be added once IPFS is included.

🚨 Test instructions

dao new
dao apps

@macor161
Copy link
Contributor Author

hmm.. there is currently a weird Travis error on node 9 and 10. I'm looking into this.

@codecov
Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #955 into develop will increase coverage by 1.12%.
The diff coverage is 22.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #955      +/-   ##
===========================================
+ Coverage    20.22%   21.35%   +1.12%     
===========================================
  Files          106      108       +2     
  Lines         2442     2440       -2     
===========================================
+ Hits           494      521      +27     
+ Misses        1948     1919      -29
Impacted Files Coverage Δ
...s/aragon-cli/src/commands/dao_cmds/utils/daoArg.js 0% <ø> (ø) ⬆️
...ckages/aragon-cli/src/lib/dao/bare-template-abi.js 100% <ø> (ø)
...li/src/commands/apm_cmds/util/generate-artifact.js 0% <0%> (ø) ⬆️
packages/aragon-cli/src/commands/dao_cmds/new.js 0% <0%> (ø) ⬆️
packages/aragon-cli/src/commands/dao_cmds/apps.js 0% <0%> (ø) ⬆️
packages/aragon-cli/src/lib/dao/new.js 100% <100%> (ø)
packages/aragon-cli/src/lib/dao/apps.js 33.33% <33.33%> (ø)
...ages/aragon-cli/scripts/setup-integration-tests.js 0% <0%> (-100%) ⬇️
... and 2 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 d70b41b...51d364b. Read the comment docs.

from: ctx.accounts[0],
gas: await getRecommendedGasLimit(web3, estimatedGas),
task: async ctx => {
console.log('registry: ', apmOptions.ensRegistryAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

👋

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch thanks! 😄

Copy link
Contributor

@kernelwhisperer kernelwhisperer left a comment

Choose a reason for hiding this comment

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

LGTM!

deployEvent,
gasPrice,
}) {
const template =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this part is a bit awkward. Is been more and more clear that we need a clear distinction of how we handle the aragonPM information. Probably something to look a bit more in detail on current iteration after we merge this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree 💯
The templateInstance was originally used here https://github.com/aragon/aragon-cli/blob/develop/packages/aragon-cli/src/commands/run.js#L371 but we'll probably be able to simplify things once we start refactoring the run command.

@dapplion
Copy link
Contributor

dapplion commented Nov 29, 2019

@macor161 According to AVA's documentation, most of the usage of t.plan() is not appropriate/necessary, see https://github.com/avajs/ava/blob/master/docs/recipes/when-to-use-plan.md

@macor161
Copy link
Contributor Author

macor161 commented Dec 1, 2019

According to AVA's documentation, most of the usage of t.plan() is not appropriate/necessary

@dapplion Ah I didn't know that thanks! It is fixed now.

@theethernaut
Copy link
Contributor

theethernaut commented Dec 2, 2019

⚠️
@macor161 it seems that dao new is not working on this PR. It seems to hang on "Fetching template..."

Also note that this might be an indication that we have a hole in the tests.

@macor161
Copy link
Contributor Author

macor161 commented Dec 2, 2019

@macor161 it seems that dao new is not working on this PR. It seems to hang on "Fetching template..."

@ajsantander hmm strange it's working here but something is clearly wrong then. I will review this furthermore.

@macor161 macor161 changed the title Extract logic from "dao apps" and "dao new" WIP Extract logic from "dao apps" and "dao new" Dec 2, 2019
@0xGabi 0xGabi self-requested a review December 4, 2019 16:25
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.

Both commands are working fine 🤝

Great work with the integration tests 🙌

@0xGabi 0xGabi changed the title WIP Extract logic from "dao apps" and "dao new" Extract logic from "dao apps" and "dao new" Dec 4, 2019
@macor161
Copy link
Contributor Author

macor161 commented Dec 4, 2019

@0xGabi Thanks a lot!! 🙂

@0xGabi 0xGabi merged commit 114dc26 into develop Dec 4, 2019
@delete-merged-branch delete-merged-branch bot deleted the sdk7-dao-new branch December 4, 2019 16:43
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

5 participants