-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Internet::Email with domain ignore the domain suffix when provided #1845
Comments
I think the method interface is quite confusing as it is. Let me point out some things that trouble me: 1 - Look at 2 - the 3 - Think about the different combinations we can have when filling domain and subdomain. Suggestion to fix: Tell me what you think, please. Thanks! @vbrazo @olleolleolle @ngouy |
I agree it's a really tricky one. This fix intened to fix it without breaking what is existing. 100% the interface should be updated |
My only concern would be this line @tiagofsilva :
What do you mean by ignore it ? I think we should keep the
I think we should preserve the dot notation the user provide in its arguments I agree it should be ignored as following : Faker::Internet.email(
name: 'jane',
domain: 'domain.com',
)
# => jane@random_subdomain.domain.com.fr Another problem I see with the solution you gave is that I have to supply an empty subdomain if I want an email with your given domain. Exemple I want Faker::Internet.email(
domain: 'my_domain',
domain_suffix: 'com',
)
# => random_name@random_subdomain.domain.com
Faker::Internet.email(
subdomain: '',
domain: 'my_domain',
domain_suffix: 'com',
)
# => random_name@my_domain.com What do you think @tiagofsilva |
what about the second part where you have to provide an empty subdomain if you don't want one @tiagofsilva ? |
Right, this is not ideal. But still better than what we have today IMO. But I have some ideas to suggest: 1 - 2 - We could orchestrate 3 - Another way to do it would be to really ignore subdomain if the param is nil. If something is passed, we concatenate to the domain name. The negative point here is that we would lose the ability to generate random subdomains as we have today. That part would be up to the user. 4 - We could allow a specific value, like I could be way off, so I think we need more opinions. Feel free to suggest them as well =). |
I agree the better option is to stick to a boolean for the subdomain (so idea 2), default to false. If not false, we put a random generated subdomain to the email Faker::Internet.email(
domain: 'my_domain',
) # => random@my_domain.random_suffix
Faker::Internet.email(
domain: 'my_domain',
domain_suffix: 'com',
) # => random@my_domain.com
Faker::Internet.email(
generate_subdomain: true,
domain: 'my_domain',
domain_suffix: 'com',
) # => random@random_subdomain.my_domain.com
Faker::Internet.email(
domain: 'my_subdomain.my_domain',
domain_suffix: 'com',
) # => random@my_subdomain.my_domain.com But indeed we have to carefully choose the var name the fourth one is too restrictive indeed and the last one just too hard to understand and use. Moreover Do we have other class api's with such options values ( |
@tiagofsilva PR is out there. Feel free to leave some comments. Even if it's not merged in the end it can be a good base to have discussions. |
Testing locally, I noticed that the was fixed at some point:
I'll see if I can figure out which version fixed it later (if no one else finds it) |
The first PR referenced in this issue fixed it (#1846) But @tiagofsilva wanted to point out the interface wans't clear enought. That what the second PR is all about |
Hey, folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the faker-ruby team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this issue. Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this issue, then please let us know so that we can reprioritize it. Thanks! |
all good, I think the issue was addressed |
#1808 introduced
domain
kw (thank you!) but does not test when a domain suffix is provied : it failsDescribe the bug
Expected behavior
when domain_suffix is proveded, keep it :
The text was updated successfully, but these errors were encountered: