Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Fix behavior when endColumn or endLine is null #1217

Merged
merged 1 commit into from
Feb 3, 2019

Conversation

mattlyons0
Copy link
Contributor

Fixes #1197, #1196, #1195, #1192

The JSDoc plugin returns null when there isn't a end column. The ESLint API docs specifically say the type of endColumn and endLine are optional or must be a number, therefore it is probably more correct and safer to do that check instead of a undefined check: https://eslint.org/docs/developer-guide/working-with-plugins

Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

Seems like a worthwhile change, thanks. I believe the tests are failing due to a logic inversion, which I've suggested a fix to below.

src/helpers.js Outdated Show resolved Hide resolved
@mattlyons0
Copy link
Contributor Author

Nice catch. I was wondering why the tests were failing on the version I was running in terminal but not the one in atom, must have missed that char when copying between. Should pass now 🤞

Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

Nice change, thanks for contributing!

@IanVS
Copy link
Member

IanVS commented Jan 7, 2019

Actually, one more thing before I merge this, do you think you could add a regression test, based on the results that the jsdoc plugin is giving? Then we'll be sure not to break this in the future.

@mattlyons0
Copy link
Contributor Author

Take a look at the test and let me know if you have any feedback. Heres the error message with the old version of helpers.js
image

@IanVS
Copy link
Member

IanVS commented Jan 11, 2019

@mattlyons0 I haven't forgotten about this, just been busy. I'll try to review in the next few days. Thanks again.

Copy link
Contributor

@skylize skylize left a comment

Choose a reason for hiding this comment

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

Thanks for your help. Please git rebase master to get in sync with the master branch. Verify everything still behaves as expected, and push the updated commit.

@mattlyons0
Copy link
Contributor Author

Looks like you are following a different commit format now, updated.

@skylize skylize merged commit 1105d26 into AtomLinter:master Feb 3, 2019
@Arcanemagus
Copy link
Member

🎉 This PR is included in version 8.5.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants