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.

Sign up for GitHub

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

Jump to bottom

Add args: --edit-page and --edit-patch #388

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add args: --edit-page and --edit-patch #388

wants to merge 1 commit into from
+133 −12

Conversation

lengyijun
Copy link

Fix #383

@lengyijun lengyijun force-pushed the edit-page branch 4 times, most recently from c54276c to b540fc9 Compare November 2, 2024 08:39
niklasmohrin
niklasmohrin requested changes Nov 9, 2024
View reviewed changes
Copy link
Collaborator

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Thanks! In addition to the comments, the added functionality is not tested by our automated tests yet. I think the tests could run tldr --edit-page foo with EDITOR=touch and the custom pages dir set to some temporary directory. Then we can observe that the correct command touch /some/tmp/dir/foo.page.md is run by checking whether the file exists or not.

Please feel free to reach out for questions, thoughts, or help :)

src/cli.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated
Comment on lines 333 to 334
let editor = env::var("EDITOR").context("env `EDITOR` not set").unwrap();
let _ = std::process::Command::new(editor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now, this does not work if people set EDITOR to something that includes arguments, for example EDITOR="nvim --clean" tldr --edit-page foo. I checked a couple of programs for how they handle this. Both git config --edit and sudoedit just spawn nvim without arguments, omitting the --clean. I suppose that having arguments in EDITOR is not a common thing then.

I think that crashing is better than omitting the arguments, so the current handling of EDITOR is fine. However, we should handle and print errors returned here.

Additionally, we should think about adding some documentation for this behavior, but let's postpone this until the code is ready.

Copy link
Author

Choose a reason for hiding this comment

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

we can use shlex to parse

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah, as I said, the current behavior is probably exactly what we want to do. As long as the behavior is documented, I think we are good.

You could add a section at the end of docs/src/usage_custom_pages.md to highlight the feature and we can phrase it so that the behavior is clear

src/main.rs Outdated Show resolved Hide resolved
@lengyijun lengyijun force-pushed the edit-page branch 5 times, most recently from 6e0b71a to a811d68 Compare November 10, 2024 03:08
niklasmohrin
niklasmohrin requested changes Dec 1, 2024
View reviewed changes
Copy link
Collaborator

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Tests look good and the flags seem to work, only some code style things left :)

src/cli.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@lengyijun lengyijun force-pushed the edit-page branch 2 times, most recently from c0bb691 to 4c25e7c Compare December 10, 2024 01:37
niklasmohrin
niklasmohrin requested changes Dec 30, 2024
View reviewed changes
Copy link
Collaborator

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

I left some more comments on the previous discussions, please let me know what you think :)

@niklasmohrin niklasmohrin added the waiting-for-author The PR needs an update before it can be considered for merging. label Jan 2, 2025
@lengyijun lengyijun force-pushed the edit-page branch 2 times, most recently from 023d886 to af896f1 Compare January 13, 2025 03:39
@niklasmohrin
Copy link
Collaborator

@lengyijun Can you address #388 (comment) ? We definitely want to have the test I described, and I think we will see that create_dir_all will not return an error

@lengyijun lengyijun force-pushed the edit-page branch 2 times, most recently from 96fb67e to 0bdc600 Compare January 14, 2025 00:38
@niklasmohrin
Copy link
Collaborator

@lengyijun you marked #388 (comment) as resolved now, but the most crucial test I brought up is still missing: when we run tealdeer a second time, we should not get an error because of trying to create the directory.

In general, I am a bit unsure on when I should take a look at the PR, please write a short comment or re-request my review on Github when you are ready :)

@lengyijun lengyijun force-pushed the edit-page branch 2 times, most recently from ab8f04b to 8c4b0cd Compare January 15, 2025 08:03
@lengyijun
Copy link
Author

Review requested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niklasmohrin niklasmohrin niklasmohrin requested changes

Assignees
No one assigned
Labels
waiting-for-author The PR needs an update before it can be considered for merging.
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add flag to open editor
2 participants
@lengyijun @niklasmohrin