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

Refactor entire codebase for separating logic from commands #858

Closed
38 of 58 tasks
theethernaut opened this issue Oct 31, 2019 · 8 comments
Closed
38 of 58 tasks

Refactor entire codebase for separating logic from commands #858

theethernaut opened this issue Oct 31, 2019 · 8 comments
Assignees

Comments

@theethernaut
Copy link
Contributor

theethernaut commented Oct 31, 2019

Planning

Planning document, here.

Following @0x6431346e's proposal on how to separate logic from commands, we're ready to start implementing this in the code.

Preliminary tasks

Having completed these preliminary tasks, we're ready to start implementing the refactor, with the following considerations:

  • Logic should be extracted to the lib/ folder following the token new command's example (Refactor token new #856).
  • Unit tests should be added to all extracted logic, again following the example.
  • We shouldn't worry about end-to-end tests for now, but do try to test commands involved with what you're changing before pushing changes.
  • Extracted logic should be 100% agnostic, and at most provide updates to the commands using them via callback handlers.
  • We can start picking off commands to refactor from the list below, and making independent PRs to the develop branch. Once you pick an issue, add your name next to the item in the list below, or ask one of the repo maintainers to add it for you.

Refactor based on commands

After refactoring the commands

  • [ Dev: , Reviewers: ] apm publish
  • [ Dev: , Reviewers: ? ] run command
  • **[ Dev: ?, Reviewers: ? ]dao commands / utils
  • Full review to see if there is still any logic within commands
  • Sanitize utilities, making sure they are all bundled nicely in the lib/ folder
  • Review unit test coverage in lib/
  • Add test on other parts of the codebase (e.g. utils)
  • Improve how we handlelistr context variables: SDK: v7 aragonone/product#222 (comment)
  • Windows CI
  • CI for boilerplates, tutorials
  • Review linter configuration and rules (@dapplion)
  • Declare yargs commands explicitly instead of with commandDir and order by priority / usage

Suggested next steps

  • Create an issue for moving the lib/ folder to a new package so that it can be bootstrapped to use Typescript
  • Create an issue to port this lib package to Typescript
@theethernaut theethernaut self-assigned this Oct 31, 2019
@sohkai sohkai mentioned this issue Oct 31, 2019
12 tasks
@0xGabi
Copy link
Contributor

0xGabi commented Oct 31, 2019

@dapplion feel free to choose from the list and comment in this issue thread to update the list☝️

@dapplion
Copy link
Contributor

dapplion commented Nov 1, 2019

@0xGabi I'll start with acl commands if it's okay.

@dapplion
Copy link
Contributor

dapplion commented Nov 2, 2019

I'll take the deploy command too

@ottodevs
Copy link

ottodevs commented Nov 5, 2019

I can take run and contract commands if they are not taken, and also I would be fine being added as a reviewer for any opened PR

@kernelwhisperer kernelwhisperer mentioned this issue Nov 6, 2019
4 tasks
@0xGabi
Copy link
Contributor

0xGabi commented Nov 7, 2019

@dapplion You want to take some of the other middleware tasks?

Note: I plan to push a PR to remove the deprecated code of the network option and init command, probably today. That will be updating a bit the environment middleware logic

@dapplion
Copy link
Contributor

dapplion commented Nov 8, 2019

@0xGabi I can tackle the rest of the middleware tasks.

@dapplion
Copy link
Contributor

dapplion commented Nov 9, 2019

@ajsantander @0xGabi You can mark the remaining middlewares as downhill

@0xGabi
Copy link
Contributor

0xGabi commented Dec 2, 2019

I consider this issue done. We tackle most of the tasks and those still not done were decided to address on #947 or move to the developer flow.

@0xGabi 0xGabi closed this as completed Dec 2, 2019
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

No branches or pull requests

4 participants