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

Refactor email generation properties #1919

Open
ST-DDT opened this issue Mar 11, 2023 · 5 comments
Open

Refactor email generation properties #1919

ST-DDT opened this issue Mar 11, 2023 · 5 comments
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: internet Something is referring to the internet module p: 1-normal Nothing urgent
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Mar 11, 2023

The email address generation function parameters are wrongly named and are lacking in some regards.

  • allowSpecialCharacters is actually forceSpecialCharacters
  • A list of allowed special characters might be more useful than a boolean flag for the end user.
  • ...

This issue is based on a discussion the team had when talking about:

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: internet Something is referring to the internet module labels Mar 11, 2023
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Mar 11, 2023
@Shinigami92 Shinigami92 moved this to Todo in Faker Roadmap Mar 13, 2023
@xDivisionByZerox
Copy link
Member

Can this be done without a breaking change?
Do we introduce the forceSpecialCharacters option in v8.0 while deprecating allowSpecialCharacters?

@matthewmayer
Copy link
Contributor

Depends on your definition of breaking change. 😀

You could argue that removing the parameter wouldn't be breaking, the docs say it "allows" special characters, not that it guarantees special characters.

@xDivisionByZerox
Copy link
Member

So what you saying is that this is more of a fix than a refactor? Or did I get you wrong?

@matthewmayer
Copy link
Contributor

I'm saying i'm pretty sure that anyone who turned this boolean on will quickly regret it as its going to generate email addresses which will fail most validators in an unpredictable way. I'd be tempted just to deprecate it (without replacement) in v8, maybe point the deprecation message to this issue, and see if anyone complains that thiis is important for their workflow?

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 13, 2023

Team Decision

We don't need this for v8.0.
(We will likely immediately start with v9 after v8.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: internet Something is referring to the internet module p: 1-normal Nothing urgent
Projects
No open projects
Status: Todo
Development

No branches or pull requests

3 participants