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

add goto next/prev error (bound to ]e/[e) #6331

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

robinvd
Copy link
Contributor

@robinvd robinvd commented Mar 16, 2023

some of my projects have a warnings from very verbose linters, this helps navigate the files when fixes errors.

Comment on lines 339 to 340
goto_next_error, "Goto next error diagnostic",
goto_prev_error, "Goto prev error diagnostic",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
goto_next_error, "Goto next error diagnostic",
goto_prev_error, "Goto prev error diagnostic",
goto_next_error, "Goto next error",
goto_prev_error, "Goto prev error",

I guess error should be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx! Ill update the pr with the suggestions when i have time

Comment on lines 2991 to 3002
let diag = match direction {
Direction::Forward => iter
.clone()
.find(|diag| diag.range.start > cursor_pos)
.or_else(|| iter.next()),
Direction::Backward => {
let mut iter = iter.rev();
iter.clone()
.find(|diag| diag.range.start < cursor_pos)
.or_else(|| iter.next())
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let diag = match direction {
Direction::Forward => iter
.clone()
.find(|diag| diag.range.start > cursor_pos)
.or_else(|| iter.next()),
Direction::Backward => {
let mut iter = iter.rev();
iter.clone()
.find(|diag| diag.range.start < cursor_pos)
.or_else(|| iter.next())
}
};
let diag = match direction {
Direction::Forward => iter
.find(|diag| diag.range.start > cursor_pos)
.or_else(|| iter.next()),
Direction::Backward => {
let mut iter = iter.rev();
iter.find(|diag| diag.range.start < cursor_pos)
.or_else(|| iter.next())
}
};

Unnecessary clone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The clone is necessary as find exhausts the iterator, thus in the suggestion or_else(|| iter.next()) will always return None

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we leave a comment here so we know it shouldn't exhaust the iterator?

Comment on lines 3004 to 3005
let selection = match diag {
Some(diag) => Selection::single(diag.range.start, diag.range.end),
Copy link
Contributor

@pickfire pickfire Mar 17, 2023

Choose a reason for hiding this comment

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

Can you please change this to let else?

    let Some(diag) = diag else { return };

@robinvd
Copy link
Contributor Author

robinvd commented Apr 11, 2023

@pickfire thx for the review! sorry about the delay. I've applied your feedback.

@cgahr
Copy link
Contributor

cgahr commented Nov 4, 2023

goto_first_error and goto_last_error is also missing, for completeness they should probably also be implemented.

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.

4 participants