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

fix(internet): username method to return value that always includes… #2506

Merged
merged 11 commits into from
Nov 19, 2023

Conversation

amillwood
Copy link
Contributor

Updated the username method to always return a value that includes the last name when provided.

Resolves #1000

@amillwood amillwood requested a review from a team as a code owner October 27, 2023 19:16
@ST-DDT ST-DDT added p: 1-normal Nothing urgent m: internet Something is referring to the internet module labels Oct 27, 2023
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #2506 (503e09e) into next (b8049d1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2506   +/-   ##
=======================================
  Coverage   99.57%   99.57%           
=======================================
  Files        2805     2805           
  Lines      250069   250088   +19     
  Branches     1097     1097           
=======================================
+ Hits       248996   249025   +29     
+ Misses       1045     1035   -10     
  Partials       28       28           
Files Coverage Δ
src/modules/internet/index.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@ST-DDT ST-DDT added this to the vAnytime milestone Oct 27, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Oct 30, 2023

Sorry for the delay in review, we will look at it shortly.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Oct 30, 2023
@amillwood
Copy link
Contributor Author

No worries. No rush, just a minor contribution - hope it helps

src/modules/internet/index.ts Outdated Show resolved Hide resolved
src/modules/internet/index.ts Outdated Show resolved Hide resolved
src/modules/internet/index.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Nov 2, 2023

One problem I see with the username method itself is that it produces very vage usernames that you might choose for private accounts.
But I think a lot of professional users of faker need usernames that companies would use and thus roughly follows the firstName.lastName strategy.

@matthewmayer
Copy link
Contributor

Companies don't always use FirstName.lastname anyway though

At companies I have worked at there have been patterns like

john.smith@
john@
jsmith@
smithj@

@ST-DDT
Copy link
Member

ST-DDT commented Nov 4, 2023

Companies don't always use FirstName.lastname anyway though

My point is that companies follow a more standardized approach to usernames than private persons do.

@matthewmayer
Copy link
Contributor

I think using both FirstName and lastname when provided makes sense, but we could have more ways to combine them for greater variety eg

More separators including empty string
Surname or firstname first
Sometimes lowercase the whole string

@ST-DDT
Copy link
Member

ST-DDT commented Nov 7, 2023

Team Decision

  • We accept this as a fix for the problem at hand, we will rethink the username method in the future.
    • (More patterns,username modes e.g. company, private, gamer...)

@ST-DDT ST-DDT added c: bug Something isn't working s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Nov 7, 2023
src/modules/internet/index.ts Outdated Show resolved Hide resolved
src/modules/internet/index.ts Outdated Show resolved Hide resolved
src/modules/internet/index.ts Outdated Show resolved Hide resolved
test/modules/internet.spec.ts Outdated Show resolved Hide resolved
test/modules/internet.spec.ts Show resolved Hide resolved
test/modules/internet.spec.ts Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Nov 9, 2023
@ST-DDT ST-DDT requested review from a team November 9, 2023 19:49
@ST-DDT
Copy link
Member

ST-DDT commented Nov 17, 2023

@amillwood Could you please fix the merge conflict?

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Nov 17, 2023
@import-brain import-brain removed the needs rebase There is a merge conflict label Nov 18, 2023
@ST-DDT ST-DDT merged commit 0ee1c67 into faker-js:next Nov 19, 2023
21 checks passed
@ST-DDT
Copy link
Member

ST-DDT commented Nov 19, 2023

Thanks for your contribution ❤️

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: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In email functionality : Second name is not appending in email most of the time
5 participants