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

included check for string literal 'error' #202

Conversation

imjordanxd
Copy link

@imjordanxd imjordanxd commented Aug 6, 2020

What is the purpose of this pull request?
Now shows an error if callback has "error" as the first param.

  • Documentation update
  • Bug fix
  • New rule
  • Changes an existing rule
  • Add autofixing to a rule
  • Other, please explain:

What changes did you make? (Give an overview)
Original if statement only checked the first param name was "err". Now it also checks for "error"

@xjamundx
Copy link
Contributor

Trying to think of any reason this might cause issues. Can you think of any? In general i think this makes sense. Might be a breaking change though.

@imjordanxd
Copy link
Author

No, I can't think of any issues. Unsure how this might become a breaking change?

@xjamundx
Copy link
Contributor

I'm fine with this goign out as a minor change, should we update the docs as well. I can probably do that later.

@jturel
Copy link

jturel commented Feb 11, 2021

Hey! This change is breaking our linting. I ask that you pardon my ignorance since I don't consider myself a JS developer, and I say that because I don't understand the significance of this change with respect to checking for a parameter called 'error' in this rule.

I'm seeing this:

/home/vagrant/katello/webpack/scenes/Tasks/helpers.js
  14:32  error  Avoid callbacks. Prefer Async/Await  promise/prefer-await-to-callbacks

The function in question is here: https://github.com/Katello/katello/blob/a7a07cb664d2de2cb56889d63c1cfcb3d4d54d41/webpack/scenes/Tasks/helpers.js#L12-L18

Changing the param from error to anything else avoids firing this rule. I have no issue with changing the param name in our code but it does seem odd, so I am sharing this here in case the rule needs adjustment.

@xjamundx
Copy link
Contributor

@jturel filed this issue to discuss #212

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.

3 participants