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 smart cli into subparsers #308

Merged
merged 28 commits into from
Jul 19, 2023
Merged

Refactor smart cli into subparsers #308

merged 28 commits into from
Jul 19, 2023

Conversation

ankona
Copy link
Contributor

@ankona ankona commented Jun 23, 2023

Separate creation of the sub-command parsers from parser execution and decouple CLI definition & instantiation.

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #308 (9f2b76c) into develop (a9018e0) will decrease coverage by 0.18%.
The diff coverage is n/a.

❗ Current head 9f2b76c differs from pull request most recent head 8cdcf84. Consider uploading reports for the commit 8cdcf84 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #308      +/-   ##
===========================================
- Coverage    87.12%   86.95%   -0.18%     
===========================================
  Files           59       59              
  Lines         3518     3518              
===========================================
- Hits          3065     3059       -6     
- Misses         453      459       +6     

see 1 file with indirect coverage changes

@ankona ankona changed the title cli update rough draft Refactor smart cli into subparsers Jul 10, 2023
@ankona ankona marked this pull request as ready for review July 10, 2023 16:03
@ankona ankona requested a review from MattToast July 10, 2023 16:04
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

Looks amazing! All of the help statements are now correct, and I love the clean up to remove the (imo unnecessary) classes and shift this to a more procedural/functional style! It should make testing much easier!!

I found a couple small knit picks, the most major being that there seems to be a type error being raised when installing for GPU. Otherwise, all other functionality seems to be working as I would expect it to!!

Thanks for the hard work on this one!!

smartsim/_core/_cli/build.py Outdated Show resolved Hide resolved
smartsim/_core/_cli/build.py Outdated Show resolved Hide resolved
smartsim/_core/_cli/build.py Show resolved Hide resolved
smartsim/_core/_cli/build.py Outdated Show resolved Hide resolved
smartsim/_core/_cli/build.py Outdated Show resolved Hide resolved
smartsim/_core/_cli/cli.py Outdated Show resolved Hide resolved
smartsim/_core/_cli/cli.py Show resolved Hide resolved
smartsim/_core/_cli/utils.py Outdated Show resolved Hide resolved
smartsim/_core/_cli/utils.py Outdated Show resolved Hide resolved
smartsim/_core/_cli/cli.py Outdated Show resolved Hide resolved
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

LGTM!! Thanks for this refactor, this should be much nicer to work with!!

@ankona ankona merged commit c7eb6d4 into CrayLabs:develop Jul 19, 2023
@MattToast MattToast added area: build Issues related to builds, makefiles, installs, etc type: refactor Issues focused on refactoring existing code labels Sep 13, 2023
@ankona ankona deleted the oss365 branch April 4, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Issues related to builds, makefiles, installs, etc type: refactor Issues focused on refactoring existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants