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

Add upholstery fabrics and colors to Vehicle provider #355

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

jaapcoomans
Copy link
Contributor

Adds colors and fabrics for upholstery to the Vehicle provider. As well as a method for a combination of both.

I needed this for a use case I'm currently implementing.

@bodiam bodiam merged commit 01cb68b into datafaker-net:main Sep 14, 2022
@bodiam
Copy link
Contributor

bodiam commented Sep 14, 2022

Thanks Jaap, you can use the snapshot version for now if you want to use it.

Also, this may have been a good case for creating your own custom, for example an upholstery faker, either by hardcoding the values or by using a yml file

@bodiam
Copy link
Contributor

bodiam commented Sep 14, 2022

Seems the test is failing btw, could you fix it?

Expecting actual:
767
"Artificial Leather"
768
to match pattern:
769
"\w+.?"

@jaapcoomans
Copy link
Contributor Author

Hi @bodiam ,

I did not mean to push it when I said I needed this change, I just meant to explain that the idea did not just come out of thin air 😄 . I have an intermediate solution now, and will switch to this when the next version is released.

I deliberately added this in the Vehicle provider, because the full domain of upholstery is much wider than just vehicles. More colors, more fabrics and probably some other things I'm not aware of. I lack the domain knowledge for that. Also, like color, upholstery is a concept that's wider than vehicles, but within the vehicle (car) domain it is a relevant part.

About the unit test: whooops! 😱 I could've sworn it succeeded locally. I must've overlooked something. Fix is on it's way!
But that makes me wonder: shouldn't failing tests block the PR being merged?

@jaapcoomans jaapcoomans deleted the add-vehicle-upholstery branch September 15, 2022 06:03
@jaapcoomans
Copy link
Contributor Author

Ah, I see now. the pattern matches a single word. This is the only value that consists of more than one word. That's why it worked locally. That's why it's flaky now. I'll correct the used pattern.

@jaapcoomans jaapcoomans mentioned this pull request Sep 15, 2022
@bodiam
Copy link
Contributor

bodiam commented Sep 15, 2022

A lot of things are random in the faker, which is why we usually use RepeatedTest, instead of Test. It just has a far higher chance of finding flaky tests like this.

And no worries about pushing things, we're pretty flexible about accepting PRs, especially small ones which don't break the API.

This is one of the reasons for starting this project, out of frustration with many other open source projects (including Java faker) which have heaps of outstanding pull requests. It's just not respectful to anyone who put time and effort into submitting a PR, hence we try to merge almost anything, and as quickly as possible.

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