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

Cli refactor #8

Merged
merged 9 commits into from
Dec 5, 2022
Merged

Cli refactor #8

merged 9 commits into from
Dec 5, 2022

Conversation

cusma
Copy link
Collaborator

@cusma cusma commented Dec 5, 2022

Description

Since the project has now major external contributors I though it was time to clean up the messy code of the initial quick&dirty CLI project.

These are the main changes:

  • CLI reorganised into 4 files: algorealm.py (the actual CLI), actions.py (game actions), query.py (blockchain data query), consts.py (useful constants);
  • Add [--test] option to the CLI to play on TestNet;
  • Switch to getpass to avoid typing mnemonic in plain text;
  • Unique claim-majesty command with mutually exclusive options (--crown or --sceptre);
  • Better TEAL contract formatting;
  • Minor changes to README.md;
  • Minor correction to AlgoRealm poem.

Things that I would like to address in different future PRs:

  • Move TEAL contracts on different repo, make them available as package to be imported in other projects (e.g. this CLI);
  • Integrate local KMD to the CLI;
  • Add [--test] option for the AlgoRealm Special Card game;
  • Add a test folder with proper tests.

Checklist

Please, make sure to comply with the checklist below before expecting review

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

@cusma cusma requested a review from aorumbayev December 5, 2022 17:31
@cusma cusma self-assigned this Dec 5, 2022
Copy link
Collaborator

@aorumbayev aorumbayev left a comment

Choose a reason for hiding this comment

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

@cusma LGTM! Nice restructuring, looks a lot cleaner now!

One quick thing though, maybe lets start versioning this and creating release tags. Since this is a minor upgrade - could you bump the version in pyproject file to 0.3.0? After that just ping me again and i'll approve - you can then create a v0.3.0 tagged release (or move it to algorealm org first and i can setup the release tag manually)

@cusma
Copy link
Collaborator Author

cusma commented Dec 5, 2022

Sure! I was planning to do it in a PR from develop to main since I would like to point source releases from main.

@cusma cusma requested a review from aorumbayev December 5, 2022 17:52
Copy link
Collaborator

@aorumbayev aorumbayev left a comment

Choose a reason for hiding this comment

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

Oh i see i didnt notice you used dev/main branches in this case. I think if we'll setup proper unit testing pipeline for this we won't need develop branch anymore, anyone can have feature branches and all testing will be solidified within pr pipelines. But we can refine it later, just a suggestion for now

@cusma cusma merged commit ca74063 into develop Dec 5, 2022
@cusma cusma deleted the cli-refactor branch December 5, 2022 17:56
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.

2 participants