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 non striped coffe blender #2377

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

ngouy
Copy link
Contributor

@ngouy ngouy commented Aug 27, 2021

Example: due to how coffee blend names are built, and the content of coffee.name_2, some blend names are not stripped, this creates some flaky test on feature test because HTML is removing trailing spaces, so when you look for a match where the reference contains a tailing space, this leads to a faulty test

image

image

Here you can see that when name_2 is randomly the empty string, then #{name_1} #{name_2} leads to a non stripped string

No-Story

Copy link
Member

@vbrazo vbrazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we need those empty spaces in the .yml file?

If there are no real reasons, I'd prefer to remove the spaces instead of doing a .strip.

@ngouy
Copy link
Contributor Author

ngouy commented Sep 2, 2021

I think @vbrazo the idea is the following: if you add "", you multiply chances of a unique coffee blender name by 2 (ish, u make single named coffee blender a thing)

I'm completely ok with what you said, it's just it will roughly divide number of unique possibilities by 2. If you are comfortable with that, I am

edit: This comment is so wrong x)
it's not n + n it's n * n so if we remove one word, impact is really low

@ngouy ngouy force-pushed the Fix-non-striped-coffee-names branch from f9ffee7 to ecf8dc9 Compare September 3, 2021 14:09
@ngouy ngouy requested a review from vbrazo September 3, 2021 14:10
@vbrazo
Copy link
Member

vbrazo commented Sep 3, 2021

@ngouy Thanks!

@vbrazo vbrazo merged commit 6a0a9d5 into faker-ruby:master Sep 3, 2021
aclemons pushed a commit to aclemons/faker that referenced this pull request Mar 2, 2022
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