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

Fixed: Quotes removal in Faker.Internet functions #212

Merged
merged 1 commit into from
Dec 30, 2018
Merged

Fixed: Quotes removal in Faker.Internet functions #212

merged 1 commit into from
Dec 30, 2018

Conversation

jc00ke
Copy link
Contributor

@jc00ke jc00ke commented Oct 26, 2018

Moved the implementation to Faker.Util.

Fixes #211

I've added:

  • CHANGELOG.md

lib/faker/util.ex Outdated Show resolved Hide resolved
lib/faker/util.ex Outdated Show resolved Hide resolved
@jc00ke
Copy link
Contributor Author

jc00ke commented Nov 13, 2018

Hi! Anything else I can add here for this?

@igas
Copy link
Member

igas commented Nov 21, 2018

Hi @jc00ke

I still think it's too specific to move to Util, I would keep it private in Internet module. I'm not sure what to suggest to test it properly. In ideal world we could test it through other public function, but we don't have one where we do not randomize it.

@jc00ke
Copy link
Contributor Author

jc00ke commented Nov 21, 2018

Yeah, I can't think of another good way to test it since the functions that use it don't take any input. About the only thing I can think of would be to make a test locale that had a single option for Name.first_name() et al with quotes in them. If not that, then I can stick it back in Internet, make it private and keep an eye out for regressions.

@igas
Copy link
Member

igas commented Dec 30, 2018

Hi @jc00ke,

I pushed a fixed test to master and updated your branch. Please take a look and tell me what you think.

Thanks.

@igas igas changed the title Changed: fixed removal of ' from Internet Fixed: Quotes removal in Faker.Internet functions Dec 30, 2018
@jc00ke
Copy link
Contributor Author

jc00ke commented Dec 30, 2018

Looks good to me. I'll keep an eye out for issues in my own test suite. I know I've seen failures lately because of ' characters but I'm pretty sure it's not from the Internet module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants