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

[NayNay] File Restructure: Base Class + Balance Restructure #207

Merged
merged 16 commits into from
Aug 5, 2024

Conversation

rh0delta
Copy link
Contributor

@rh0delta rh0delta commented Jul 31, 2024

Related Issue(s)

Proposed Changes

  • 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

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed

Screenshots (if applicable)

Additional Context

Checklist

  • I have performed a self-review of my code.
  • I have added tests.
  • I have commented my code.
  • I have included a CHANGELOG.md entry.
  • I have updated documentation in github.com:entropyxyz/entropy-docs, where necessary.

- 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
@rh0delta rh0delta linked an issue Jul 31, 2024 that may be closed by this pull request
@frankiebee
Copy link
Collaborator

frankiebee commented Jul 31, 2024

notes from draft verbal review:
the base class should just be called Base
lets drop controller as a naming scheme possibly use command instead if you want to be verbose

@rh0delta rh0delta marked this pull request as ready for review July 31, 2024 20:26
src/balance/utils.ts Outdated Show resolved Hide resolved
src/balance/utils.ts Outdated Show resolved Hide resolved
@mixmix
Copy link
Contributor

mixmix commented Aug 1, 2024

I'm assuming this doesn't need more review from me @frankiebee ?

Anyone feel free to tag me back in if you want attention

@rh0delta
Copy link
Contributor Author

rh0delta commented Aug 1, 2024

@mixmix would love your thoughts on this one, specifically the new file structure and the organization/use of the new files

@rh0delta
Copy link
Contributor Author

rh0delta commented Aug 1, 2024

@frankiebee updated the utils to be pure functions, wanna re-review 😬

@mixmix
Copy link
Contributor

mixmix commented Aug 1, 2024

REVIEW INBOUND

Copy link
Contributor

@mixmix mixmix left a comment

Choose a reason for hiding this comment

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

Good start to to restructure. I think we need that flows/balance folder gone to see how this is gonna land. I think this might end up then touching the root cli.ts and tui.ts.

And then the whole session concept ....phew

CHANGELOG.md Outdated Show resolved Hide resolved
src/flows/balance/cli.ts Outdated Show resolved Hide resolved
Comment on lines 3 to 4
import * as BalanceUtils from "./utils"
import { FLOW_CONTEXT } from "./constants"
Copy link
Contributor

Choose a reason for hiding this comment

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

Questions:

  1. why did you split out utils?
    • is it because you don't want logger things in that level?
  2. why is FLOW_CONTEXT extracted?
    • it seems like that's not gonna be used by anything else, so ... premature optimization
    • can you just put it as a const at the top of this file / in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Questions:

  1. why did you split out utils?

    • is it because you don't want logger things in that level?
  2. why is FLOW_CONTEXT extracted?

    • it seems like that's not gonna be used by anything else, so ... premature optimization
    • can you just put it as a const at the top of this file / in the constructor?
  1. 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.
  2. 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`
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

rh0delta and others added 6 commits July 31, 2024 22:20
Co-authored-by: mix irving <mix@protozoa.nz>
…ance on every command; used to be passed down to each of the commands; added new option to define new address as selected account
@rh0delta rh0delta requested a review from mixmix August 1, 2024 19:54
@rh0delta rh0delta mentioned this pull request Aug 2, 2024
8 tasks
@rh0delta
Copy link
Contributor Author

rh0delta commented Aug 5, 2024

@mixmix got some time for a re-review on this one?

@frankiebee frankiebee merged commit 3b6d981 into naynay/file-restructure Aug 5, 2024
2 checks passed
@frankiebee frankiebee deleted the naynay/base-plus-balance branch August 5, 2024 17:17
@github-actions github-actions bot locked and limited conversation to collaborators Aug 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

balance restructure
3 participants