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

Remove defaults from image module #2314

Closed
ST-DDT opened this issue Aug 14, 2023 · 6 comments · Fixed by #2472
Closed

Remove defaults from image module #2314

ST-DDT opened this issue Aug 14, 2023 · 6 comments · Fixed by #2472
Assignees
Labels
breaking change Cannot be merged when next version is not a major release c: feature Request for new feature good first issue Good for newcomers m: image Something is referring to the image module p: 1-normal Nothing urgent
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Aug 14, 2023

Clear and concise description of the problem

Currently most of the image module's functions have defaults for their options.
These defaults originate from the legacy faker project and are arbitrarily chosen.

Suggested solution

Remove the defaults and make them random to increase the variety of our generated data.
The options should remain optional.

Alternative

Keep it as is.

Additional context

Related:

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent breaking change Cannot be merged when next version is not a major release m: image Something is referring to the image module labels Aug 14, 2023
@xDivisionByZerox xDivisionByZerox added the good first issue Good for newcomers label Sep 24, 2023
@olrtg
Copy link
Contributor

olrtg commented Oct 11, 2023

I would like to work on this later this day!

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 11, 2023

I would like to work on this later this day!

Thanks. Please note that this is schedulled for v9.0 and thus might take a while before it can be merged (At least 2 weeks).

@olrtg
Copy link
Contributor

olrtg commented Oct 11, 2023

Sure! no worries. And please note that I may not be able to work on this today 😆 but will try!

@olrtg
Copy link
Contributor

olrtg commented Oct 12, 2023

Hey! two questions. What about urlLoremFlickr and urlPicsumPhotos? Both have a default width/height also. Should I randomize those two too?

And the other question is, what method should I use to randomize those values? Do you want to use arrayElements? What would be the set of values? 🤔

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 12, 2023

What about urlLoremFlickr and urlPicsumPhotos? Both have a default width/height also. Should I randomize those two too?

Yes. Please.

And the other question is, what method should I use to randomize those values? Do you want to use arrayElements? What would be the set of values? 🤔

I would probably go for faker.number.int between 1px and 4k. We can discuss the exact values when we have the PR.

@olrtg
Copy link
Contributor

olrtg commented Oct 12, 2023

Great! thanks for the quick reply. Will work on that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: feature Request for new feature good first issue Good for newcomers m: image Something is referring to the image module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants