-
Notifications
You must be signed in to change notification settings - Fork 443
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
feat: adds tutor config edit
#1099
base: main
Are you sure you want to change the base?
feat: adds tutor config edit
#1099
Conversation
tutor/commands/config.py
Outdated
@click.command(name="edit", help="Edit config.yml of the current environment") | ||
@click.pass_obj | ||
def edit(context: Context) -> None: | ||
config_file = os.path.join(context.root, "config.yml") |
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.
Use config.config_path(context.root)
instead.
tutor/commands/config.py
Outdated
elif which("start"): # Windows | ||
open_cmd = ["start", '""', config_file] | ||
else: | ||
click.echo( |
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 command should fail if neither open commands are available. You could for instance raise exceptions.TutorError
.
tutor/commands/config.py
Outdated
|
||
open_cmd = None | ||
|
||
if which("open"): # MacOS |
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.
Let's amend the comment. open
is also available on Ubuntu, for instance.
I have another, more important problem with this command. According to my testing, when I run open file.txt
on a headless server (for instance: a remote production server), it opens a read-only editor. How can we fix this command to open a write-capable editor? (xdg-open
is not available on servers without a Desktop) Note that $EDITOR
is set to "vim" on this server.
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.
@regisb I wanted to test this scenario to evaluate the options. However, the server image I used (ubuntu based) didn't have a open
at all. And my desktop with Ubuntu seems to just ship xdg-open
as open
.
❯ which open
/usr/bin/open
❯ open --version
xdg-open 1.1.3
when I run
open file.txt
on a headless server (for instance: a remote production server), it opens a read-only editor.
Which version of open
is this, and what's the editor that it opens?
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.
On the topic of a "writable editor", MacOS's open
implements -a
flag, which allows specifying the application to open the file. I am wondering if something similar can be added to this command, that can force a specific editor to be launched. tutor config edit -a vim
maybe? What do think about this?
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 think this can be a good option, this gives control to the user. For instance, if someone wants to open the file in Sublime/VScode, they can override the default utils the command is checking.
tutor/commands/config.py
Outdated
open_cmd = ["start", '""', config_file] | ||
else: | ||
click.echo( | ||
"Cannot find a way to open the editor automatically. Kindly open the file 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.
Let's rephrase that along the lines of: "Failed to detect an adequate text file editor". (mode concise, no need for "kindly")
@regisb Hi, Thanks for the review comments. I hoped to have addressed them by now. Somehow haven't found the time. I will handle the comments this week. |
I have nothing to add beyond Regis's review--just want to say that this is a great idea and thank you for the contribution 👍🏻 |
ecf0b53
to
853e51e
Compare
tutor/commands/config.py
Outdated
elif which("start"): # Windows | ||
open_cmd = ["start", '""', config_file] | ||
else: | ||
raise exceptions.TutorError(f"Failed to find utility to launch an editor.") |
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.
In the error message, we can specify which utilities are used by the command internally so that user can install them on the system if needed (not all the users of Tutor are dev, some are operators who might not want to delve into code to see what is happening under the hood).
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.
After some research, it seems to me that Windows will always have start
, macOS will always have open
, and Linux desktop environments will always have xdg-open
. That leaves only non-desktop Linux users and perhaps BSD users. I think those users will know their systems better then we will, and they will be capable of figuring out how to open a yaml file as long as we tell them where it is 😄 @tecoholic , rather than letting them know what to install, could you just enhance this error message a bit just to let users know that they can edit configuration at {config_file}
?
853e51e
to
c095e45
Compare
Hi @tecoholic . I'm looking forward to this change! Looks like there are just a couple minor change requests left, and then we're good to merge this. |
@kdmccormick Hi, thanks for the ping. I was waiting on @regisb for more details about read only editor. Can we skip that scenario for now? |
@kdmccormick I am less interested in this feature than you seem to be -- mostly because (IMHO) it doesn't bring anything more than a .bashrc alias. E.g: |
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.
Thanks for the patience @tecoholic . If you can rebase and address these comments, I'll be happy to merge this.
tutor/commands/config.py
Outdated
elif which("start"): # Windows | ||
open_cmd = ["start", '""', config_file] | ||
else: | ||
raise exceptions.TutorError(f"Failed to find utility to launch an editor.") |
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.
After some research, it seems to me that Windows will always have start
, macOS will always have open
, and Linux desktop environments will always have xdg-open
. That leaves only non-desktop Linux users and perhaps BSD users. I think those users will know their systems better then we will, and they will be capable of figuring out how to open a yaml file as long as we tell them where it is 😄 @tecoholic , rather than letting them know what to install, could you just enhance this error message a bit just to let users know that they can edit configuration at {config_file}
?
c095e45
to
2b00211
Compare
@kdmccormick Thank you for your comments and your patience with this PR. I have addressed your comments to my best. Kindly take another look when you can. |
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.
Please add a changelog entry for this change.
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 tested locally, works great on Ubuntu! I also commented out the which("open")
block in order to confirm that xdg-open
also works.
In addition to Dawoud's request to add a changelog entry, I have just one more change request and one optional suggestion ⬇️
@@ -255,9 +259,41 @@ def patches_show(context: Context, name: str) -> None: | |||
print(rendered) | |||
|
|||
|
|||
@click.command(name="edit", help="Edit config.yml of the current environment") |
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.
Optional: What do you think about adding a -s/--save
flag here? If supplied, then instead of reminding the user to run tutor config save
, it would just save config automatically.
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 was something I thought of adding later based on user feedback. Now that you have pointed it out, I have taken it as the feeback and added 2 features I think are QOL improvments.
-s
automatically updating the environment after a successful save. The caveat here is "successful save". More on them below.-e
to specify an editor of choice. The annoying thing about desktop systems is, installing new software somehow messes up file associations. I discovered after installing KDE that my YAML files now open in Kate Editor. Now I dotutor config edit -e (vim|emacs|..)
to open the file in a specific editor.
Save caveats:
- Sometimes, when I do
:wq
in Neovim (Lazyvim config), it returns a non-zero error code. This prevents the "save" from happening. - When the file gets opened in a GUI editor like Kate, it opens it as an independent process, instead of subprocess. So the
utils.execute()
returns as soon as the editor is launched instead of return after the editor is closed. So, the environment is updated with config before editing.
Quickly launch the default YAML editor for editing the config.yml file.
Co-authored-by: Kyle McCormick <[email protected]>
Co-authored-by: Kyle McCormick <[email protected]>
00565bb
to
d3cd4c0
Compare
@kdmccormick Thank you for the feedback. I added a changelog entry, new options |
<!-- | ||
Create a changelog entry for every new user-facing change. Please respect the following instructions: | ||
- Indicate breaking changes by prepending an explosion 💥 character. | ||
- Prefix your changes with either [Bugfix], [Improvement], [Feature], [Security], [Deprecation]. | ||
- You may optionally append "(by @<author>)" at the end of the line, where "<author>" is either one (just one) | ||
of your GitHub username, real name or affiliated organization. These affiliations will be displayed in | ||
the release notes for every release. | ||
--> | ||
|
||
<!-- - 💥[Feature] Foobarize the blorginator. This breaks plugins by renaming the `FOO_DO` filter to `BAR_DO`. (by @regisb) --> | ||
<!-- - [Improvement] This is a non-breaking change. Life is good. (by @billgates) --> |
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.
please remove the default text from changelog.
|
||
<!-- - 💥[Feature] Foobarize the blorginator. This breaks plugins by renaming the `FOO_DO` filter to `BAR_DO`. (by @regisb) --> | ||
<!-- - [Improvement] This is a non-breaking change. Life is good. (by @billgates) --> | ||
[Feature] Adds `tutor config edit`. This opens the active tutor environment's config.yaml in an editor for manual editing. (by @tecoholic) |
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.
nit: Please add -
at the start so that it renders as a list item.
tutor/commands/config.py
Outdated
"-e", | ||
"--editor", | ||
default="", | ||
help="Editor to open the config.yaml file.", |
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.
nit: the help text should mention if this is name or path to editor (or both).
@DawoudSheraz I have addressed your comments about the docs and updated the PR. |
Quickly launch the default YAML editor for editing the
config.yml
file.Background
As a developer working with multiple projects, I am constantly changing the tutor config and re-running the
tutor dev launch
to update the devstack. While it is as simple as runningvim $(tutor config printroot)/config.yml
, I often found myself wanting to simply express this better using tutor config edit. So this is an attempt at cross-platform solution doing the same. It might not be the best solution, but something to start with if someone else finds this useful and improves their DevEx.Caveat: I have only tested this on Linux (both open and
xdg-open
works in mine, so it could be said Unix).