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

Allow specifying file start position #445

Merged
merged 1 commit into from
Jan 23, 2022

Conversation

pickfire
Copy link
Contributor

Like helix-term/src/commands.rs:3426:15

@pickfire pickfire requested a review from archseer July 14, 2021 15:59
@pickfire pickfire force-pushed the file-pos branch 2 times, most recently from b1cb72e to 722cdaa Compare July 14, 2021 16:42
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-term/src/args.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
@pickfire pickfire force-pushed the file-pos branch 3 times, most recently from c1c5486 to 5f1abac Compare July 20, 2021 11:36
helix-term/src/args.rs Outdated Show resolved Hide resolved
helix-term/src/args.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
Comment on lines +113 to +115
if limit_before_line_ending {
row = row.min(text.len_lines() - 1);
};
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary? It's handled a few lines lower

Copy link
Contributor Author

@pickfire pickfire Nov 23, 2021

Choose a reason for hiding this comment

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

Yes, or otherwise it will panic when it is out of bounds.

@Rational-Curiosity
Copy link
Contributor

Linux allows : in the file name. With this feature, how to open a file named helix-term/src/commands.rs:3426:15 from the command line?

@cole-h
Copy link
Contributor

cole-h commented Nov 21, 2021

EDIT: Oh, I see your point now.

Then I suppose we should probably check if there's a file with the same name before starting to parse it for line and column.

@Rational-Curiosity
Copy link
Contributor

Rational-Curiosity commented Nov 21, 2021

EDIT: Oh, I see your point now.

Then I suppose we should probably check if there's a file with the same name before starting to parse it for line and column.

Then... How to open a file named helix-term/src/commands.rs:3426:15 existing other file named helix-term/src/commands.rs?

@cole-h
Copy link
Contributor

cole-h commented Nov 21, 2021

There's no perfect solution. Even for your PR of +line:column, what if I have a file named exactly +10:10? How do I open file asdf first, alongside +10:10 without going to line 10 and column 10 of file asdf?

Do you find yourself editing files named test:10:10 alongside a file named test that often? To me, this sounds like an edge case that should be acknowledged, but not supported.

IMHO, I'd much rather support this syntax because that's what almost every compiler outputs in their diagnostics.

@Rational-Curiosity
Copy link
Contributor

Rational-Curiosity commented Nov 21, 2021

There's no perfect solution. Even for your PR of +line:column, what if I have a file named exactly +10:10? How do I open file asdf first, alongside +10:10 without going to line 10 and column 10 of file asdf?

Linux way: hx -- +10:10 opens a file named +10:10.
hx -- +10:10 asdf opens two files named +10:10 and asdf. hx +10:10 asdf opens asdf at point 10, 10.

Do you find yourself editing files named test:10:10 alongside a file named test that often? To me, this sounds like an edge case that should be acknowledged, but not supported.

I wrote an example, there are others: How to open a new file named helix-term/src/commands.rs:3426:15?
I have found applications that store old logs with the time at the end, for example: file.log2021-11-21_08:03:01. I think helix is awesome, why to start with annoying behaviors? it isn't necessary. Can you imagine a new user trying to open a file named file.log2021-11-21_08:03:01 and not understanding what happens?

IMHO, I'd much rather support this syntax because that's what almost every compiler outputs in their diagnostics.

This problem has been well solved by other applications. I believe that it is convenient to take advantage of their experience. For example, reading compiler output with helix and implementing file_at_point function that supports file.ext:10:10 format.

@pickfire
Copy link
Contributor Author

pickfire commented Nov 23, 2021

I am aware of this but I haven't encounter it myself.

@Curiosidad-Racional please choose better wording, you made it sound like I purposely write this patch to annoy users.

My original thoughts was check if it exists before splitting, but I think since file extension is usually at the back rather than the syntax here so I think it won't be an issue.

Or maybe I should just add the check before splitting it?

@Rational-Curiosity
Copy link
Contributor

Rational-Curiosity commented Nov 24, 2021

I am aware of this but I haven't encounter it myself.

@Curiosidad-Racional please choose better wording, you made it sound like I purposely write this patch to annoy users.

As you may have noticed I'm not a native English speaker. My intention was not to offend but to clarify. I was referring to annoying behaviors:

  1. opening a file named file.log2021-11-21_08:03:01
  2. creating a file named file.log2021-11-21_08:03:01

I didn't refer to the PR. Altruistic contributions are admirable.

My original thoughts was check if it exists before splitting, but I think since file extension is usually at the back rather than the syntax here so I think it won't be an issue.

IMO users espect hx [filename] to open literal [filename]. Other behaviors are usually with modifiers hx --compiler <filename:line:col>. This is an example, I don't pretend that it is implemented like this... I'm giving ideas not solutions.

Or maybe I should just add the check before splitting it?

Whatever you prefer. We just don't make a possible use of helix impossible (1. or 2.).

:-)

@pickfire
Copy link
Contributor Author

I implemented a check before splitting.

helix-term/src/args.rs Outdated Show resolved Hide resolved
@@ -56,7 +56,7 @@ USAGE:
hx [FLAGS] [files]...

ARGS:
<files>... Sets the input file to use
<files>... Sets the input file to use or with pos file[:row[:col]]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<files>... Sets the input file to use or with pos file[:row[:col]]
<files>... Sets the input file to use, position can also be specified via file[:row[:col]]

Copy link
Contributor Author

@pickfire pickfire Nov 29, 2021

Choose a reason for hiding this comment

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

Screenshot_20211129_225244

Not very nice looking on small terminals.

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Looks like there's a few merge conflicts

helix-term/src/args.rs Outdated Show resolved Hide resolved
Like helix-term/src/commands.rs:3426:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open file on line/col
5 participants