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

enable starting hx with a working directory #8223

Merged
merged 15 commits into from
Oct 3, 2023

Conversation

lloydbond
Copy link
Contributor

@lloydbond lloydbond commented Sep 9, 2023

You can now use -w or --working-dir to set the current working directory of the hx editor from command line launch.

Similar to these other editors here
example:
hx -w <path> --log hx.log -- <files>...
hx --working-dir <path> --log hx.log -- <files>...
hx -w <path> --log hx.log
hx --log hx.log <path>
confirm it works by checking your current working directory from inside the editor with :pwd
confirm the files are opened with :bn

@pascalkuthe pascalkuthe added the R-breaking-change This PR is a breaking change for some behavior label Sep 9, 2023
@pascalkuthe
Copy link
Member

pascalkuthe commented Sep 9, 2023

I don't think this need to be builtin, you could just do cd {project} && hx {file}. I don't think this needs to be builtin, we try to keep the argument parsing minimal

Ah I think I get this now the description is a bit confusing, so all you allow is also opening files in the background if opening a directory (which is already supported). That seems alright

@pascalkuthe pascalkuthe added E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer. and removed R-breaking-change This PR is a breaking change for some behavior labels Sep 9, 2023
@lloydbond
Copy link
Contributor Author

Yes, That is correct. This improves hx's ability to be triggered as an external editor.

Also I can close this PR

@archseer
Copy link
Member

Using a regular parameter and making this order dependent seems wrong for me and potentially confusing for ens users. Why not just use a proper flag?

@lloydbond
Copy link
Contributor Author

Hi @archseer, it was mentioned in the matrix chat that the developers of Helix-Term are trying to avoid adding flags. You can see in a comment I made above that I mention closing a PR in favor of this PR, that PR does use a flag for setting the initial workspace directory. Either or both approaches is fine with me.

@archseer
Copy link
Member

On Matrix I was specifically talking about configuration flags that would be put in config.toml, not CLI flags.

I'd prefer this be a flag rather than a parameter

@gabydd
Copy link
Member

gabydd commented Sep 11, 2023

I think that was me that said we try not too add many command line options, I think based on some comments by mike in other issues, though from what I just read know those where about not having command line options that could be config options. From what I remember from matrix it seems like the reason for this was because there was use case where cd couldn't be used? If cli flag is preferred that is good with me 👍

@lloydbond
Copy link
Contributor Author

Okay, do we want to close this PR and reopen this PR with the -w flag? I'm good with either.

@archseer
Copy link
Member

Why not just update the current PR by force pushing? No need to open a PR each time the approach is changed, that way the conversation isn't fragmented

@lloydbond
Copy link
Contributor Author

Sure, that works. Let me make the updates.

@lloydbond lloydbond force-pushed the open_workspace_from_cli branch from bbbbf8b to 5f62202 Compare September 16, 2023 00:17
bhainesva added a commit to bhainesva/helix that referenced this pull request Sep 26, 2023
add @variable.other.member query for keyed elements
remove special case of all caps vars treated as const

fixes helix-editor#8223
helix-term/src/main.rs Outdated Show resolved Hide resolved
helix-term/src/args.rs Outdated Show resolved Hide resolved
helix-term/src/args.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
@lloydbond
Copy link
Contributor Author

@pascalkuthe I updated the code on line 172 to have

if let Some(path) = args.working_directory {
            helix_loader::set_current_working_dir(path)?
        }

Then I realized, If there are no files passed, then the -w flag would get completely ignored and no working directory would be set. This could be okay, but I think not an expected behavior from a normal user's perspective. For now, I have moved the code snippet above to line 159.

This means that hx -w <path> and hx <path> are functionally the same.
Also hx -w <path1> -- <path2> <file1> <file2>
would follow these steps

  1. set the working directory to path1
  2. set the working directory to path2 like normal
  3. open the fuzzy file finder window like normal
  4. ignore file1 and file2 like normal
    Unless there is some other intended behavior that is preferred, I think this is the better option for now.

Other options could be:

  1. original solution with the code on line 172 and an additional else if check to test if we have -w set but args.files is empty (not my favorite and complicates things).
  2. we could go with the original solution and a note in the --help message for -w flag that mentions, -w does nothing if no files are passed with it.
  3. original solution only, and when -w silently does nothing, let the user figure out they should have gone with hx <path> and not hx -w <path>

Let me know what you think. Hopefully this doesn't overly complicate things.

pascalkuthe
pascalkuthe previously approved these changes Sep 30, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

I think k the way you implemented it now makes sense and is quite simple. I prefer to keep the cli simple 👍

@the-mikedavis the-mikedavis requested a review from archseer October 1, 2023 18:48
@archseer archseer merged commit 75c0a5c into helix-editor:master Oct 3, 2023
6 checks passed
danillos pushed a commit to danillos/helix that referenced this pull request Nov 21, 2023
* added working path arg to cli and help menu

* improve working path cli arg handling

* enable hx to set the working path

* applied cargo formatting

* improved code from cargo clippy suggestion

* improved code from follow up review

* fix for -w <path> is set but args.files is empty

* improved formatting of --help output
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
* added working path arg to cli and help menu

* improve working path cli arg handling

* enable hx to set the working path

* applied cargo formatting

* improved code from cargo clippy suggestion

* improved code from follow up review

* fix for -w <path> is set but args.files is empty

* improved formatting of --help output
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
* added working path arg to cli and help menu

* improve working path cli arg handling

* enable hx to set the working path

* applied cargo formatting

* improved code from cargo clippy suggestion

* improved code from follow up review

* fix for -w <path> is set but args.files is empty

* improved formatting of --help output
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
* added working path arg to cli and help menu

* improve working path cli arg handling

* enable hx to set the working path

* applied cargo formatting

* improved code from cargo clippy suggestion

* improved code from follow up review

* fix for -w <path> is set but args.files is empty

* improved formatting of --help output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants