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

Make it mandatory to also provide references when creating new validators? #2110

Closed
pano9000 opened this issue Dec 4, 2022 · 1 comment · Fixed by #2161
Closed

Make it mandatory to also provide references when creating new validators? #2110

pano9000 opened this issue Dec 4, 2022 · 1 comment · Fixed by #2161

Comments

@pano9000
Copy link
Contributor

pano9000 commented Dec 4, 2022

Hello all,

Just wanted to bring this topic up, because I can't seem to find any rules about this yet:

I feel like it would be good practice to have the people who create a PR also provide some (credible) references for any new validators or additions to existing validators.

I.e. what information is the validation based on?

Those should ideally of course be some official docs, and not solely based on a (possibly outdated) Wikipedia entry.
I can see some PRs already providing this info, which is great and make it a lot easier to check, but then there are other ones, that do not.
In the latter case, you will just see a random RegExp that is supposed to check for something, and you won't know, is this valid or not.

There can be several reasons, where errors could result in a wrong RegExp, e.g.

  • based on outdated info
  • potentially misinterpreting or misreading the way the string is made up
  • accidentally making the RegExp "too permissive"

In those cased the (mandatory) provided tests of course can also be "faulty" as well.

Yes, providing references might make the PR process a tad bit more cumbersome, but I would rather say it is better to go for quality over quantity, especially for a validation library.

Implementing this will come with some benefits

  • it'll make it easier for people to actually review what the RegExp is supposed to test for
  • it'll increase confidence in the library doing correct validations

So I would propose to add this as an additional point to the "PR Checklist" at the very least.

What are your thoughts?

@braaar
Copy link
Contributor

braaar commented Dec 7, 2022

I wholeheartedly agree. Good suggestion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants