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

Provide accurate error messages when jumping to columns #364

Merged
merged 3 commits into from Feb 22, 2019

Conversation

Projects
None yet
3 participants
@50Wliu
Copy link
Member

50Wliu commented Jan 27, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

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

This comment has been minimized.

Copy link
Contributor

jtagscherer commented Jan 27, 2019

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

This comment has been minimized.

Copy link
Contributor

rafeca commented Feb 15, 2019

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

await getOrScheduleUpdatePromise()
expect(bufferView.element.querySelectorAll('li').length).toBe(0)
spyOn(bufferView, 'moveToCaretPosition').andCallThrough()
atom.commands.dispatch(bufferView.element, 'core:confirm')

@rafeca rafeca force-pushed the wl-line-column-message branch from 955fd9f to a24e860 Feb 22, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

rafeca commented Feb 22, 2019

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

@rafeca rafeca merged commit c3c81d0 into master Feb 22, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@50Wliu

This comment has been minimized.

Copy link
Member Author

50Wliu commented Feb 22, 2019

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

@50Wliu 50Wliu deleted the wl-line-column-message branch Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.