[NayNay] File Restructure: Base Class + Balance Restructure #207
Conversation
- created new base entropy class to handle the shared nature of the intended use of the class - restructured balance to be in its own parent directory in the root of src - created command and util file to contain pure functions and controller functions
|
notes from draft verbal review: |
…updated changelog
|
I'm assuming this doesn't need more review from me @frankiebee ? Anyone feel free to tag me back in if you want attention |
|
@mixmix would love your thoughts on this one, specifically the new file structure and the organization/use of the new files |
|
@frankiebee updated the utils to be pure functions, wanna re-review 😬 |
|
REVIEW INBOUND |
src/balance/command.ts
Outdated
| import * as BalanceUtils from "./utils" | ||
| import { FLOW_CONTEXT } from "./constants" |
There was a problem hiding this comment.
Questions:
- why did you split out utils?
- is it because you don't want logger things in that level?
- why is
FLOW_CONTEXTextracted?- it seems like that's not gonna be used by anything else, so ... premature optimization
- can you just put it as a
constat the top of this file / in the constructor?
There was a problem hiding this comment.
Questions:
why did you split out utils?
- is it because you don't want logger things in that level?
why is
FLOW_CONTEXTextracted?
- it seems like that's not gonna be used by anything else, so ... premature optimization
- can you just put it as a
constat the top of this file / in the constructor?
- splitting out utils was a decision made to have one unified file that contains the pure functions for the individual command, and remove any idea about having to do anything but make the request to retrieve balance (in this case) and return that data. the removal of logging here was a decision made to allow the command class take care of the "business logic" as my corporate colleagues would call it, which includes error handling/logging and logging of generic information.
- yeah it was kinda premature, i was under the impression there would be more shareable variables, and originally there was a use since we had the need for flow context in both the utils and command classes, but that has since changed so i will update and remove this file
|
|
||
| this.logger.log(`Current balance of ${address}: ${balance}`, `${BalanceCommand.name}`) | ||
|
|
||
| return `${balance.toLocaleString('en-US')} BITS` |
There was a problem hiding this comment.
I really don't think this should have BITS, I don't even know about the tolocaleString.
We want this to be CLI first, so just raw JSON friendly formats like numbers, objects, not sentences (number + unit).
I could be wrong!
There was a problem hiding this comment.
the utils method is the pure function that returns a number, my vision was that the controller/command file will handle how we want the data to be presented
Co-authored-by: mix irving <mix@protozoa.nz>
…xyz/cli into naynay/base-plus-balance
…ance on every command; used to be passed down to each of the commands; added new option to define new address as selected account
… removed flows/balance directory
|
@mixmix got some time for a re-review on this one? |
Related Issue(s)
Proposed Changes
Testing
Screenshots (if applicable)
Additional Context
Checklist
CHANGELOG.mdentry.github.com:entropyxyz/entropy-docs, where necessary.