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.

Sign up for GitHub

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

Jump to bottom

fix(internet): fix invalid emails in some locales #1746

Merged
merged 8 commits into from
Jan 20, 2023
Merged

fix(internet): fix invalid emails in some locales #1746

merged 8 commits into from
Jan 20, 2023

Conversation

matthewmayer
Copy link
Contributor

fix #1739

@matthewmayer matthewmayer requested a review from a team as a code owner January 17, 2023 05:12
@codecov
Copy link

codecov bot commented Jan 17, 2023 •
edited
Loading

Codecov Report

Merging #1746 (70034e6) into next (dfa647d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #1746      +/-   ##
==========================================
+ Coverage   99.63%   99.64%   +0.01%     
==========================================
  Files        2337     2337              
  Lines      242582   242589       +7     
  Branches     1098     1103       +5     
==========================================
+ Hits       241698   241737      +39     
+ Misses        863      831      -32     
  Partials       21       21
Impacted Files Coverage Δ
src/locales/el/person/last_name.ts 100.00% <100.00%> (ø)
src/locales/fa/person/last_name.ts 100.00% <100.00%> (ø)
src/modules/internet/index.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 91.12% <0.00%> (+8.60%) ⬆️

@ST-DDT
Copy link
Member

ST-DDT commented Jan 17, 2023

FFR: This fixes only one of two problems in #1739 .

@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Jan 17, 2023
@ST-DDT ST-DDT added c: bug Something isn't working p: 2-high Fix main branch m: internet Something is referring to the internet module s: accepted Accepted feature / Confirmed bug labels Jan 17, 2023
@ST-DDT ST-DDT mentioned this pull request Jan 17, 2023
Closed
@matthewmayer
Copy link
Contributor Author

FFR: This fixes only one of two problems in #1739 .

do you have an example of an invalid email address which is still generated?

@matthewmayer
Copy link
Contributor Author

remove names with () in them

ah looks like that was the problem in el which i fixed in this commit 58dfab4

@matthewmayer matthewmayer changed the title fix(internet): limit email address local part to 64 chars fix(internet): fix invalid emails in some locales Jan 17, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Jan 17, 2023

Can you also add a filter that removes all invalid characters from the local part, so it still generates a valid email even if the given name/firstname are invalid? Please also add a test for both too long names and invalid characters.

@ST-DDT ST-DDT added the needs test More tests are needed label Jan 17, 2023
@ST-DDT ST-DDT removed the needs test More tests are needed label Jan 18, 2023
ST-DDT
ST-DDT previously approved these changes Jan 18, 2023
View reviewed changes
src/modules/internet/index.ts Show resolved Hide resolved
@ST-DDT ST-DDT requested review from a team January 18, 2023 09:11
import-brain
import-brain previously approved these changes Jan 18, 2023
View reviewed changes
@matthewmayer matthewmayer dismissed stale reviews from import-brain and ST-DDT via 28bc97c January 19, 2023 13:04
Shinigami92
Shinigami92 approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

LGTM

@Shinigami92 Shinigami92 merged commit 1e4e869 into faker-js:next Jan 20, 2023
@xDivisionByZerox xDivisionByZerox mentioned this pull request Jan 21, 2023
Merged
@matthewmayer matthewmayer deleted the next branch January 30, 2023 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xDivisionByZerox xDivisionByZerox xDivisionByZerox approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@ST-DDT ST-DDT ST-DDT approved these changes

@import-brain import-brain import-brain left review comments

Assignees

@matthewmayer matthewmayer

Labels
c: bug Something isn't working m: internet Something is referring to the internet module p: 2-high Fix main branch s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.0 - Module Re-Shuffling
Development

Successfully merging this pull request may close these issues.

Generated email addresses are sometimes invalid in certain locales
5 participants
@matthewmayer @ST-DDT @Shinigami92 @import-brain @xDivisionByZerox