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 input validation #7

Closed
wants to merge 8 commits into from

Conversation

lilianchiassai
Copy link

Fixes 2 issues reported in this ticket

  • input fields were validated for each rule applied to the parent input, leading to multiple validation of the same object if multiple rules were applied to the parent input. Now, all rules are validated against the input parent before validating individually each input field.
  • using inputs as mutation or query argument would only result in the validation of the directive applied to the argument itself. This is now fixed.

@bbakerman
Copy link
Member

I suspect this PR has some great value in it - however since it re-wrote all the spaces and import order and so on its really hard to see whats actually changed

Finally the fact this has ZERO tests does not inspire me to accept it since how can we know it fixed anything?

@bbakerman
Copy link
Member

I do appreciate open source contributions but they have to have tests and they should try to show only code changes

@lilianchiassai
Copy link
Author

Hello sir!

Sorry about that. I'll try to rework that PR when I can. The formatting and testing comes from my ide/project not using the same auto formatting nor the same test framework (I don't know how to build tests with the framework included in this project), combined to a lack of time to dedicate to that.

If you want to work on the ticket related to this PR yourself, feel free! Otherwise I'll clean this up, but it might not be in the near future because I'm not using this library anymore.

@bbakerman bbakerman closed this Sep 16, 2020
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.

2 participants