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

fix(require-param): check deeply destructured parameters (fixes #569) #630

Merged
merged 11 commits into from
Sep 7, 2020

Conversation

hyex
Copy link
Contributor

@hyex hyex commented Sep 5, 2020

fixes #569

Check the type of destructured parameters in the require-param rule.

@hyex hyex changed the title fix(require-param): check deeply destructured parameters (fixes #569) fix(require-param): check deeply destructured parameters (fixes #569) Sep 5, 2020
@brettz9
Copy link
Collaborator

brettz9 commented Sep 5, 2020

Looks great from a quick look, thank you very much!

In case it will all need to be redone though, I'm a little reluctant to add much more code on top of our code until the first part of #540 might be addressed (i.e., "Where a root and grandchildren are present but not children, the fixing order is off."). Do you think this is something you could also look into?

If not, I can still see about merging as it of course is better to have something working.

@hyex
Copy link
Contributor Author

hyex commented Sep 5, 2020

Thank you for your quick response!

Actually, It's my first PR in open source. So #540 seems like a difficult issue for me to solve for now... Can I try it when I have enough time?

@brettz9
Copy link
Collaborator

brettz9 commented Sep 6, 2020

Ok--but as we also have an checkTypesPattern option for check-param-names, could you at least see if that needs an equivalent fix (and if so, ideally make the fix there also)?

@hyex
Copy link
Contributor Author

hyex commented Sep 6, 2020

Of course!
I also agree that I need to fix check-param-names. I will ping you after finishing the fix.

The error mentioned in issue gajus#569 also occurred in `check-param-name` rule. So I fixed.
@hyex
Copy link
Contributor Author

hyex commented Sep 6, 2020

@brettz9
Hi, I fixed what you mentioned.
Please let me know if there are need any changes. I will be appreciated any review. 😊

@brettz9 brettz9 merged commit 428174d into gajus:master Sep 7, 2020
@brettz9
Copy link
Collaborator

brettz9 commented Sep 7, 2020

Great work, thank you!!

@gajus
Copy link
Owner

gajus commented Sep 7, 2020

🎉 This PR is included in version 30.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants