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

Accept +num flag for opening at line number #5603

Closed
wants to merge 1 commit into from

Conversation

nabaco
Copy link
Contributor

@nabaco nabaco commented Jan 20, 2023

Resolves #5437
Added basic logic for opening files with +N argument, where N is the line number.
Can be mentioned after each file. If mentioned several times, latest one will be taken.

This is a very basic implementation, since in Vim/NeoVim the + arguments runs a command in the editor.
With some help, I can try to change the implementation. Need to understand how I can use the execute() method upon startup. (helix-term/src/commands.rs:164)

P.S: I'm new to Rust, so any comments about my code are more than welcome :)

@CBenoit CBenoit added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 20, 2023
@nabaco
Copy link
Contributor Author

nabaco commented Jan 20, 2023

Need to fix Clippy warnings. I'll do that, probably after the weekend.
I've managed to do an implementation where + executes any command, but it doesn't work in the form of +11. I'm suspecting Shellwords::from(&str) is swallowing the numbers for some reason.

Anyone willing to help me with this?

@nabaco
Copy link
Contributor Author

nabaco commented Jan 20, 2023

I've pushed my second implementation as a second commit, for review and discussion. In the second implementation, +11 doesn't work but +'open README.md', +'goto 11', and +'g 11' work like a charm.
+q panics 😞 but I don't think it's important right now since I don't see +q being a very important or common usecase.

Anyway, any comments, suggestions, reviews are more than welcome.

Copy link
Contributor Author

@nabaco nabaco left a comment

Choose a reason for hiding this comment

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

Here is the issue. I'm missing the number in +11 since it's the first element. Need to either add an if here or insert "goto" into command

@nabaco
Copy link
Contributor Author

nabaco commented Jan 20, 2023

Fixed the numeric command issue in the final commit. Tested with the following commands:

$ ./target/debug/hx README.md +11
$ ./target/debug/hx README.md +'g 11'
$ ./target/debug/hx README.md +'goto 11'
$ ./target/debug/hx +'open README.md' +11
$ ./target/debug/hx +'open README.md' +'theme acme'

helix-term/src/args.rs Outdated Show resolved Hide resolved
helix-term/src/args.rs Outdated Show resolved Hide resolved
helix-term/src/main.rs Outdated Show resolved Hide resolved
@pascalkuthe
Copy link
Member

Maybe this was already discussed (altough I didn't find anything) but what about using :open <path>:<line>:<column> here? You could reuse the parse_file function and would stay more consisntent with the CLI and also gf if something like #5260 is merged

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 21, 2023
@nabaco
Copy link
Contributor Author

nabaco commented Jan 22, 2023

@pascalkuthe that was mentioned in the original issue: #5437.
I'll move the execution part to a separate PR. I agree that it somewhat a hack and needs better design.

@nabaco nabaco force-pushed the plus-goto-argument branch from 9575ffd to eaea712 Compare January 23, 2023 18:47
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2023
@gibbz00 gibbz00 mentioned this pull request Jan 24, 2023
5 tasks
@@ -17,6 +17,7 @@ pub struct Args {
pub log_file: Option<PathBuf>,
pub config_file: Option<PathBuf>,
pub files: Vec<(PathBuf, Position)>,
pub line_number: usize,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this isn't used outside of the parse_args function so it could be a local in that function: let mut line_number = 0;

Comment on lines +79 to +85
args.line_number = match arg.parse() {
Ok(n) => n,
_ => anyhow::bail!("bad line number after +"),
};
if args.line_number > 0 {
args.line_number -= 1;
}
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
args.line_number = match arg.parse() {
Ok(n) => n,
_ => anyhow::bail!("bad line number after +"),
};
if args.line_number > 0 {
args.line_number -= 1;
}
args.line_number = match arg.parse::<usize>() {
Ok(n) => n.saturating_sub(1),
_ => anyhow::bail!("bad line number after +"),
};

@@ -73,6 +73,7 @@ FLAGS:
-V, --version Prints version information
--vsplit Splits all given files vertically into different windows
--hsplit Splits all given files horizontally into different windows
+N Goto line number N
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
+N Goto line number N
+N Open the first given file at line number N

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 18, 2023
@@ -73,6 +74,16 @@ impl Args {
}
}
}
arg if arg.starts_with('+') => {
let arg = arg.get(1..).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This can also be cleaned up a little: &arg[1..]

pascalkuthe pushed a commit that referenced this pull request Oct 12, 2023
* Accept +num flag for opening at line number

* Update +N argument feature according to feedback in original PR #5603

* Only override the line number of the first file if +N is specified

---------

Co-authored-by: Nachum Barcohen <38861757+nabaco@users.noreply.github.com>
@kirawi
Copy link
Member

kirawi commented Oct 12, 2023

Closed by #8521

@kirawi kirawi closed this Oct 12, 2023
danillos pushed a commit to danillos/helix that referenced this pull request Nov 21, 2023
…#8521)

* Accept +num flag for opening at line number

* Update +N argument feature according to feedback in original PR helix-editor#5603

* Only override the line number of the first file if +N is specified

---------

Co-authored-by: Nachum Barcohen <38861757+nabaco@users.noreply.github.com>
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
…#8521)

* Accept +num flag for opening at line number

* Update +N argument feature according to feedback in original PR helix-editor#5603

* Only override the line number of the first file if +N is specified

---------

Co-authored-by: Nachum Barcohen <38861757+nabaco@users.noreply.github.com>
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
…#8521)

* Accept +num flag for opening at line number

* Update +N argument feature according to feedback in original PR helix-editor#5603

* Only override the line number of the first file if +N is specified

---------

Co-authored-by: Nachum Barcohen <38861757+nabaco@users.noreply.github.com>
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
…#8521)

* Accept +num flag for opening at line number

* Update +N argument feature according to feedback in original PR helix-editor#5603

* Only override the line number of the first file if +N is specified

---------

Co-authored-by: Nachum Barcohen <38861757+nabaco@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept +num flag for opening at line number
5 participants