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

chore/eslint-update #107

Merged
merged 6 commits into from
Jan 24, 2020
Merged

chore/eslint-update #107

merged 6 commits into from
Jan 24, 2020

Conversation

busticated
Copy link
Contributor

No description provided.

return Boolean(this.auth);
}

/**
* Get firmware library objects
* @param {Object} query The query parameters for libraries. See Particle.listLibraries
* @return {Promise}
* @returns {Promise} A promise
Copy link
Contributor Author

@busticated busticated Jan 23, 2020

Choose a reason for hiding this comment

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

a bit silly since we should be specifying exactly what the promise is resolved & rejected with but it's enough to placate the linter and not any worse than what we were doing

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 40.643% when pulling 4f54b44 on chore/eslint-update into 17fac8f on master.

*/
downloadFirmwareBinary({ binaryId, auth, context }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wondering if this should be used here..? something like:

this.request({ uri, method: 'get', context });

* emit 'event' events.
*/
getEventStream({ deviceId, name, org, product, auth, context }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another unused context instance - this one less controversial since i don't think it's relevant here?

*/
createIntegration({ integrationType, event, settings, deviceId, product, auth, context }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another unused variable - flagging this one since the option was also documented

*/
deleteLibrary({ name, version, force }) {

Choose a reason for hiding this comment

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

This makes me, hmmm

Undocumented argument, gets removed, doesn't cause any other changes in the diff unless I missed them, so maybe not a tested method. Just want to make sure this is alright?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not being used so i think it's safe 👍

Copy link

@wraithan wraithan left a comment

Choose a reason for hiding this comment

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

I'm concerned about the deleteLibrary change but aside from that the changes look routine for applying stronger linting rules and updating them.

@monkbroc
Copy link
Member

deleteLibrary is largely unused since it's not possible to delete a public library, only an unpublished private library.

@busticated busticated merged commit 3dad0ee into master Jan 24, 2020
@busticated busticated deleted the chore/eslint-update branch January 24, 2020 00:47
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