Skip to content

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

Merged
Shinigami92 merged 8 commits into
faker-js:nextfrom
matthewmayer:next
Jan 20, 2023
Merged

fix(internet): fix invalid emails in some locales#1746
Shinigami92 merged 8 commits into
faker-js:nextfrom
matthewmayer:next

Conversation

@matthewmayer

Copy link
Copy Markdown
Contributor

fix #1739

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

codecov Bot commented Jan 17, 2023

Copy link
Copy Markdown

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%) ⬆️
Comment thread src/modules/internet/index.ts Outdated
@ST-DDT

ST-DDT commented Jan 17, 2023

Copy link
Copy Markdown
Member

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
@matthewmayer

Copy link
Copy Markdown
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
Copy Markdown
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

ST-DDT commented Jan 17, 2023

Copy link
Copy Markdown
Member

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
Comment thread src/modules/internet/index.ts
@ST-DDT ST-DDT requested review from a team January 18, 2023 09:11
ejcheng
ejcheng previously approved these changes Jan 18, 2023
Comment thread src/modules/internet/index.ts Outdated
Comment thread test/internet.spec.ts
@matthewmayer matthewmayer dismissed stale reviews from ejcheng and ST-DDT via 28bc97c January 19, 2023 13:04

@Shinigami92 Shinigami92 left a comment

Copy link
Copy Markdown
Member

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
@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

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

5 participants