atom / fuzzy-finder

Find and open files quickly
MIT License
276 stars 138 forks source link

Provide accurate error messages when jumping to columns #364

Closed 50Wliu closed 5 years ago

50Wliu commented 5 years ago

Requirements

Description of the Change

With #360, you can now specify a specific column when jumping to a line. The error reporting for invalid line numbers, however, was not updated to allow columns as well. This PR adds additional cases to the error handling such that it reports column errors correctly and does not inadvertently display "Invalid line number" when attempting to navigate to a column.

Alternate Designs

None.

Benefits

An incorrect error is removed and there is better discoverability of the feature.

Possible Drawbacks

None.

Applicable Issues

Fixes #363, /cc @jtagscherer

jtagscherer commented 5 years ago

Works great, thanks! 👍

I was actually just fixing this as well, and I pulled out the errorMessage and emptyMessage into variables that I set in the individual branches of the if statement. That way the readability of the code is improved a bit and I have to call this.selectListView.update only once. That may be an option to consider:

var emptyMessage = null
var errorMessage = null

if (/^:\d+:\d*\D/.test(query)) {
  errorMessage = 'Invalid column number'
} else if (/^:\d+:/.test(query)) {
  emptyMessage = 'Jump to line and column in active editor'
} else if (/^:\d*\D/.test(query)) {
  errorMessage = 'Invalid line number'
} else {
  emptyMessage = 'Jump to line in active editor'
}

this.selectListView.update({
  items: [],
  emptyMessage: emptyMessage,
  errorMessage: errorMessage
})

Oh, and a quick heads-up: You committed your package-lock.json, which you probably shouldn't.

rafeca commented 5 years ago

Looks great! You can add an assertion to check that the message is correct around the following lines:

https://github.com/atom/fuzzy-finder/blob/94b91a7fb2a5d9f9eaf6d2d17e71eb5420c9acf3/spec/fuzzy-finder-spec.js#L916-L920

rafeca commented 5 years ago

I've just added some tests, we can merge this now! 🎉

50Wliu commented 5 years ago

Thanks @rafeca! Was a bit busy so I wasn't able to get around to finishing this.