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

Sanitise fake email domain #53

Closed
wants to merge 2 commits into from
Closed

Sanitise fake email domain #53

wants to merge 2 commits into from

Conversation

jezen
Copy link

@jezen jezen commented May 3, 2024

In order to more closely conform to RFC 1035 and actually generate valid fake emails, we only allow alphanumeric characters or hyphens in the text output.

See: https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.1

Resolves #52

In order to more closely conform to RFC 1035 and actually generate valid
fake emails, we only allow alphanumeric characters or hyphens in the
text output.

See: https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.1

Resolves #52
Copy link
Member

@psibi psibi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can you look into the test failures ? Updating to use the last 3 lts is fine if fixing it is hard.

Also, can you add a test to not allow underscore ?

@jezen
Copy link
Author

jezen commented Aug 5, 2024

Sorry, I haven't had time to come back to this, and now I can't see why CI failed because it's beyond the log retention period. I've sent an empty commit just to trigger CI.

@Oleksandr-Zhabenko
Copy link
Contributor

I have forked the main branch and updated the stack configuration so that now it supports the last three stable LTS Stack branches.

I also noticed the random tests failures with countries in the test/AddressSpec.hs file. I have provided a commentary in the file with possible source of failures.

The nightly branch is constantly failing because of dependencies. It is generally considered unstable. Can we omit it and think that the three last LTS branches are supported?

@psibi
Copy link
Member

psibi commented Aug 7, 2024

The nightly branch is constantly failing because of dependencies. It is generally considered unstable. Can we omit it and think that the three last LTS branches are supported?

Sounds good to me.

@Oleksandr-Zhabenko
Copy link
Contributor

So may I create a pull request to update the dependencies?

@Oleksandr-Zhabenko
Copy link
Contributor

The source of possible random failures in address can be occasionally deleted data-files from the fakedata.cabal in the previous commits. Now I added the data back and see no failures.

@psibi
Copy link
Member

psibi commented Aug 7, 2024

So may I create a pull request to update the dependencies?

Yes.

@psibi
Copy link
Member

psibi commented Aug 12, 2024

@jezen The CI is fixed in the main branch, I think rebasing it will help.

@Oleksandr-Zhabenko
Copy link
Contributor

Oleksandr-Zhabenko commented Aug 12, 2024

@psibi since the CI is now updated, I can try to integrate the changes proposed by @jezen into the repository. Or, probably, you can do it.

@psibi
Copy link
Member

psibi commented Aug 13, 2024

@Oleksandr-Zhabenko I will leave the decision to @jezen since he is the author of this PR.

@jezen
Copy link
Author

jezen commented Aug 13, 2024

I'm happy for @Oleksandr-Zhabenko to take this work over from me. Thanks for checking! 🙂

@Oleksandr-Zhabenko
Copy link
Contributor

Ok, I will take this for me.

@Oleksandr-Zhabenko
Copy link
Contributor

I have applied changes from the @jezen pull request and also provided additional check in the domain name for no underscores and the first mandatory letter.
https://github.com/Oleksandr-Zhabenko/fakedata/actions/runs/10369246394/job/28704567097
The changes are according to:
https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.1

@Oleksandr-Zhabenko
Copy link
Contributor

The following address has more extended email validation pattern. Is it better to use it?
https://stackoverflow.com/questions/201323/how-can-i-validate-an-email-address-using-a-regular-expression

@Oleksandr-Zhabenko
Copy link
Contributor

Oleksandr-Zhabenko commented Aug 13, 2024

This one also uses some improvements.
https://github.com/Oleksandr-Zhabenko/fakedata/actions/runs/10369246394

@psibi
Copy link
Member

psibi commented Aug 13, 2024

@Oleksandr-Zhabenko Nice! I would avoid using regex. I think the minimal check proposed by jezen is good enough.

@Oleksandr-Zhabenko
Copy link
Contributor

Oleksandr-Zhabenko commented Aug 13, 2024

Should I make a fallback to the variant without introduced by me checking of underscores and hyphens?

psibi pushed a commit that referenced this pull request Aug 15, 2024
* Update stack.yaml

* Update stack.yaml

* Changed yaml files to use string-random-0.1.4.3

* Changed yaml files to use text-2.0.2

* Changed yaml files

* Changed yaml files

* Changed yaml files

* Changed yaml files

* Changed yaml files

* Changed package.yaml file

* Changed yaml files

* Added stack-lts-2* yaml files

* Changed workflow yaml file

* Changed workflow yaml file

* Changed workflow yaml file

* Changed workflow yaml file

* Changed stack.yaml file

* Changed stack.yaml file

* Changed stack.yaml file

* Changed stack.yaml file

* Added commentary to test/AddressSpec.hs about the reason of random tests failures

* Added commentary to test/AddressSpec.hs about the reason of random tests failures

* Changed stack.yaml

* Added missed data-files in fakedata.cabal

* Accepted changes proposed by psibi

* Accepted changes proposed by psibi for stack.yaml

* Accepted changes proposed by psibi for stack.yaml

* Fixed .github/workflows/tests.yml

* Added dist/cabal-config-flags to .gitignore file

* Removed unneeded binary files

* Removed formatting and the commentary

* Sanitized fake email domain

* Added hyphen into domain name checker in test/TextSpec.hs

* Changed domain name checker in test/TextSpec.hs
@jezen
Copy link
Author

jezen commented Aug 15, 2024

Closing this in favour of #56.

@jezen jezen closed this Aug 15, 2024
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.

Invalid domain names generated
3 participants