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

Contributed EmailAddress Scalar #22

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

Juke-Duke
Copy link

This is an attempt on adding specification for an EmailAddress Scalar. I made a library for .NET, ScalarKit, implementing common Scalar types for developers to utilize in their domains, and am trying a crack on making it GraphQL compliant. Any feedback is appreciated 🙏🏽

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 8, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@andimarek
Copy link
Collaborator

Hi @Juke-Duke ... thanks a lot for your contribution.

Let me explain a bit how the graphql scalars repo work, as this is a very new project and I want to make everybody is on the same page.

First: you or your company have to sign the CLA. The bot message above #22 (comment) has more details. This is a formality to ensure some legal aspects are covered.

About the contribution process: we are only really reviewing the format of your scalars spec and make sure all formals aspects are covered. You own the actual spec itself. Of course we can give recommendations or we can discuss the spec, if you like us to chime in, but ultimately you own the spec and you decide how it should look like.

Please be aware that once the spec is merged it can't change anymore. So we will not allow any semantic changes to it, but we will allow clarifications or typos. The consequence is that if you change your mind you have to create a new custom scalars spec. e.g. email-address-2 or so.

Let me know if you have any questions.

@dondonz
Copy link
Collaborator

dondonz commented Feb 8, 2023

To add to @andimarek's comment, when you are ready with your specification, please request review from myself or @andimarek.

@Juke-Duke
Copy link
Author

Hey @andimarek and @dondonz . Thank you so much for the explanation, makes things much clearer. I would definitely not mind some input, as I really enjoy using GraphQL, and I am a bit of a perfectionist 😆 , so I really want to make sure the Scalar is industry ready. I have signed the CLA and am awaiting anymore feedback. Thanks again!

@Juke-Duke
Copy link
Author

Hey @andimarek , @dondonz , does anything need to be refactored to best make this Scalar industry standard? I am happy to receive any input on this, thanks!

@andimarek
Copy link
Collaborator

Hey @Juke-Duke ... It looks fine from my end regarding the form and general style. I am happy to merge it anytime you are happy with it. (remember: you are responsible for the actual content and it is your call at the end when to merge it)

One comment regarding the actual semantics: I like it that you already simplified it and restrict the email to the most useful formats. Maybe you could even go further and disallow: admin@mailserver1. Probably almost all APIs don't want to allow this kind of value. But then again this might make the regex more complicated.

Also: can you provide a reference why this regex matches the simplified email you describe? I think that would be helpful for people.

@cgriego
Copy link

cgriego commented Jun 7, 2024

I would recommend adopting HTML's definition of a valid email address which is used for email inputs.
https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address

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.

5 participants