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

Static Random Image Support #1

Merged
merged 2 commits into from
Jul 5, 2020
Merged

Static Random Image Support #1

merged 2 commits into from
Jul 5, 2020

Conversation

h3xexe
Copy link
Contributor

@h3xexe h3xexe commented Jul 3, 2020

I think its a good option to have :)

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2020

Codecov Report

Merging #1 into master will increase coverage by 3.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master       #1      +/-   ##
============================================
+ Coverage     85.71%   88.88%   +3.17%     
- Complexity       17       21       +4     
============================================
  Files             1        1              
  Lines            42       54      +12     
============================================
+ Hits             36       48      +12     
  Misses            6        6              
Impacted Files Coverage Δ Complexity Δ
src/PicsumProvider.php 88.88% <100.00%> (+3.17%) 21.00 <4.00> (+4.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b8e8fb...d19b3cf. Read the comment docs.

@h3xexe h3xexe marked this pull request as draft July 3, 2020 03:43
@h3xexe h3xexe closed this Jul 3, 2020
@h3xexe h3xexe reopened this Jul 3, 2020
@h3xexe h3xexe force-pushed the master branch 9 times, most recently from 41b98bd to dab9d59 Compare July 3, 2020 06:42
@h3xexe h3xexe marked this pull request as ready for review July 3, 2020 06:48
@morawskim
Copy link
Owner

hello @hasanalibalcioglu,

Yeap, I agree with you. This is a good option. I have thought about adding a new parameter to the method. Currently, the id parameter allows us to fetch a specific photo. The randomize argument append-only some random number to URL to prevent fetch image from a cache by a browser. If We add a new method, we don't need to check if the user provides randomize or static argument. This will be more clear for the user. We can move creating a query string to a separate method to prevent duplication. Also for static images, we don't need a randomize argument.

@h3xexe h3xexe force-pushed the master branch 5 times, most recently from 20e2684 to 64892e2 Compare July 4, 2020 01:34
@h3xexe
Copy link
Contributor Author

h3xexe commented Jul 4, 2020

Thank you for directives. As you can see i dont have much experience with tests and contributing. Is it ok now?

@h3xexe h3xexe force-pushed the master branch 3 times, most recently from 1a64faa to 83941d6 Compare July 4, 2020 01:44
Copy link
Owner

@morawskim morawskim left a comment

Choose a reason for hiding this comment

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

I think this PR is ok, but Can you rename the method?

src/PicsumProvider.php Outdated Show resolved Hide resolved
@morawskim morawskim merged commit d6e573f into morawskim:master Jul 5, 2020
@morawskim
Copy link
Owner

Thanks @hasanalibalcioglu

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.

None yet

3 participants