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.

Sign up for GitHub

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

Jump to bottom

refactor(image)!: rewrite module #1477

Merged
merged 34 commits into from Dec 5, 2022
Merged

refactor(image)!: rewrite module #1477

merged 34 commits into from Dec 5, 2022

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Oct 22, 2022 •
edited

closes #1350

Changes:

  • Deprecate all image providers
  • Deprecate image module category methods
  • Changed url method to accept options with provider
  • Methods now accepts options

@Shinigami92 Shinigami92 added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs breaking change Cannot be merged when next version is not a major release m: image Something is referring to the image module labels Oct 22, 2022
@Shinigami92 Shinigami92 self-assigned this Oct 22, 2022
@Shinigami92 Shinigami92 linked an issue Oct 22, 2022 that may be closed by this pull request
Rewrite image module / providers #1350
Closed
@codecov
Copy link

codecov bot commented Oct 28, 2022 •
edited

Codecov Report

Merging #1477 (ad2acb2) into next (4ed45fa) will decrease coverage by 0.00%.
The diff coverage is 98.15%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #1477      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2222     2222              
  Lines      239816   240423     +607     
  Branches     1052     1064      +12     
==========================================
+ Hits       238926   239521     +595     
- Misses        869      881      +12     
  Partials       21       21
Impacted Files Coverage Δ
src/modules/image/providers/lorempicsum.ts 91.66% <87.75%> (-2.20%) ⬇️
src/modules/image/providers/lorempixel.ts 92.48% <95.12%> (+1.17%) ⬆️
src/modules/image/index.ts 100.00% <100.00%> (ø)
src/modules/image/providers/placeholder.ts 100.00% <100.00%> (ø)
src/modules/image/providers/unsplash.ts 100.00% <100.00%> (ø)

@Shinigami92 Shinigami92 marked this pull request as ready for review October 28, 2022 19:00
@Shinigami92 Shinigami92 requested a review from a team as a code owner October 28, 2022 19:00
ST-DDT
ST-DDT previously approved these changes Oct 28, 2022
View reviewed changes
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Oct 28, 2022
@ST-DDT ST-DDT requested review from a team October 28, 2022 21:08
@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Nov 20, 2022
import-brain
import-brain reviewed Nov 23, 2022
View reviewed changes
test/image.spec.ts Show resolved Hide resolved
test/image.spec.ts Show resolved Hide resolved
test/image.spec.ts Show resolved Hide resolved
test/image.spec.ts Show resolved Hide resolved
test/image.spec.ts Show resolved Hide resolved
import-brain
import-brain previously approved these changes Nov 25, 2022
View reviewed changes
@Shinigami92 Shinigami92 dismissed stale reviews from import-brain and ST-DDT via ea3a4d0 November 25, 2022 16:14
ST-DDT
ST-DDT requested changes Nov 26, 2022
View reviewed changes
src/modules/image/index.ts Outdated Show resolved Hide resolved
src/modules/image/index.ts Show resolved Hide resolved
src/modules/image/index.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2022

This should be mentioned in the upgrade docs.

@ST-DDT ST-DDT added the needs documentation Documentations are needed label Nov 26, 2022
Shinigami92 and others added 2 commits November 26, 2022 17:27
@Shinigami92 @ST-DDT
chore: apply review suggestion
455ec6b 
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@Shinigami92 @ST-DDT
chore: apply review suggestion
7aca603 
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2022

I think you have to run the formatter once.

ST-DDT
ST-DDT approved these changes Nov 27, 2022
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

I still think avatar should only point to GH avatars but otherwise this is fine.

@Shinigami92
Copy link
Member Author

@ST-DDT I will take you out as co-author, even if you have worked with me and there are applied review suggestion.
I'm doing this as if there are really any actual law concerns, you are not under fire and I'm responsible for taking changes to the legacy data set.

@Shinigami92 Shinigami92 merged commit 616b34d into next Dec 5, 2022
@Shinigami92 Shinigami92 deleted the 1350-rewrite-image-module branch December 5, 2022 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT approved these changes

@import-brain import-brain import-brain approved these changes

@xDivisionByZerox xDivisionByZerox Awaiting requested review from xDivisionByZerox

Assignees

@Shinigami92 Shinigami92

Labels
breaking change Cannot be merged when next version is not a major release c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: image Something is referring to the image module needs documentation Documentations are needed p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.0 - Module Re-Shuffling
Development

Successfully merging this pull request may close these issues.

Rewrite image module / providers
3 participants
@Shinigami92 @ST-DDT @import-brain