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

ExtractInputType: show all excess args in offense #32

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

bessey
Copy link
Contributor

@bessey bessey commented Mar 3, 2021

I believe this makes it clearer what is wrong when introducing the cop to an existing code base, where a mutation may already have an excess of arguments.

Otherwise, you delete an argument, the errors moves up a line, and so on until you get below MaxArguments.

Just a draft for now with no spec improvements to confirm you're open to PRs! Happy to fix up anything that's broken, which may be everything haha.

@bessey
Copy link
Contributor Author

bessey commented Mar 4, 2021

Clearly my pot shot edit through GH doesn't work 😆 happy to clone and do it the right way if you are open to a PR

@DmitryTsepelev
Copy link
Owner

Hi @bessey, I'm definitely open to PRs and suggestions 🙂

@DmitryTsepelev DmitryTsepelev added the waiting for response Further information is requested label Mar 28, 2021
@bessey
Copy link
Contributor Author

bessey commented Apr 1, 2021

I will look to get to this in my next sprint, FYI

@bessey bessey marked this pull request as ready for review April 7, 2021 15:16
@bessey
Copy link
Contributor Author

bessey commented Apr 7, 2021

Does this look good to you now @DmitryTsepelev?

I believe this makes it clearer what is wrong when introducing the cop to an existing code base, where a mutation may already have an excess of arguments.

Otherwise, you delete an argument, the errors moves up a line, and so on until you get below MaxArguments.
@DmitryTsepelev
Copy link
Owner

LGTM, thank you so much!

@DmitryTsepelev DmitryTsepelev merged commit 2c1a3e0 into DmitryTsepelev:master Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for response Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants