-
Notifications
You must be signed in to change notification settings - Fork 34
Don't default pdstools CLI -> let user choose between health_check and decision_analyzer
#395
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
Conversation
…health_check and decision analyzer
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #395 +/- ##
==========================================
- Coverage 64.22% 64.00% -0.23%
==========================================
Files 58 58
Lines 5470 5489 +19
==========================================
Hits 3513 3513
- Misses 1957 1976 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Good idea. It does bring the DA app more to the front, so let’s address cosmetics soon (after Friday?). The intro, empty sections here and there, an “init” page (showing in both apps now), marketing your optimization feature more by giving it a better name - etc etc. There’s also a few warnings and semi random print statements in the DA app. Let’s do those things soon. |
| run_parser = subparsers.add_parser("run", help="Run the specified pdstools app") | ||
| run_parser.add_argument( | ||
| "app", | ||
| choices=["health_check", "decision_analyzer"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than the nerdy lowercased underscored names, why not use "Health Check" and "Decision Analyzer"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so that you don't have to quote these arguments when giving them as command line arguments (i.e., pdstools run decision_analyzer vs pdstools run "Decision Analyzer". We still need that shortcut for programmatic use - the 'options' thing is just for convenience. but perhaps we could use an alias when choosing from options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alias sounds good
| args.func(args, unknown) | ||
|
|
||
|
|
||
| def run(args, unknown): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this run method both in py.typed and cli.py? Looks pretty identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
py.typed is a completely empty file, no? it just indicates to mypy that the code is typed, doesn't actually contain anything. where are you seeing anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. It's empty - I must have seen it wrong before.
|
I am a bit confused, it is giving me "No module named streamlit" error even after installing it |
Yeah, I don't really know how that works either. I do know that uvx uses a temporary virtual environment and doesn't use your |
|
@jmelburg , is this something you can try to look at? The use case here is that it would be great if we could simply run |
@yusufuyanik1 , @operdeck, any concerns with this? I would like to ultimately be able to run
uvx pdstoolsand have you choose between health check and decision analyzer -> then you don't even need to pre-install pdstools, just uv. Please test this out to see if it works like you would like.