-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: improve the commands output #8
Conversation
5ffe821
to
1787fae
Compare
6ad0fa1
to
add7f2c
Compare
add7f2c
to
0934700
Compare
@@ -80,6 +81,7 @@ def run_command(command: str) -> None: | |||
Args: | |||
command (str): Tutor command. | |||
""" | |||
click.echo(tutor_fmt.command(command)) |
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.
I don't want this to be a blocker since the design of running multiple commands is out of the scope of this PR, so please, don't consider it one.
Why can't we use tutor_utils.execute
instead of directly calling subprocess.Popen
? It will save us a lot of trouble since we won't have to handle errors or echo the command info manually.
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.
I tried, but it makes the chained elements weird.
For example:
Using: tutor_utils.execute(*command.split())
it returns:
tutor plugins update '|' tutor plugins index add https://raw.githubusercontent.com/eduNEXT/tutor-plugin-indexes/picasso_test/
To make it possible to allow chained elements, we would need to add shell=True in https://github.com/overhangio/tutor/blob/master/tutor/utils.py#L216
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.
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.
I used this solution. Having the sh -c prefix is not good looking but improves the readability of the code and doesn't bother a lot in the logs.
If you have more feedback about this, please let me know.
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.
This looks significantly better and gives even more information for debugging purposes. I tested it and it's working as expected. Thank you!
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.
Hi @MaferMazu, thanks for the PR!
Could we show in the cover letter the before and after the commands output?
b740f9a
to
c3e69e4
Compare
9900b37
to
0d46c8d
Compare
f659497
to
8673780
Compare
Thanks, @mariajgrimaldi @BryanttV, for the feedback. I applied it. I would love your re-review. |
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.
LGTM!
Description
This PR uses
tutor_utils.execute()
in theenable-private-packages
andenable-themes
commands to make debugging easier when they are inside theextra-commands
command.Pd: I couldn't use
tutor_utils.execute()
inextra-commands
, because it uses chained commands.More info about that util: https://github.com/overhangio/tutor/blob/v18.1.3/tutor/utils.py#L210
Also, check for themes to be dictionaries to avoid the
type ignore
comments.How to test
tutor picasso run-extra-commands
with this branch using theenable-themes
andenable-private-packages
inside. (See the screenshots for this test).Screenshots
Using in the config.yml:
Before
After
After merge