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

Change method userReportMake in ClientInterface #17

Merged
merged 28 commits into from
Oct 13, 2020
Merged

Change method userReportMake in ClientInterface #17

merged 28 commits into from
Oct 13, 2020

Conversation

gomzyakov
Copy link
Contributor

@gomzyakov gomzyakov commented Oct 8, 2020

Description

Added

  • Class ReportMakeParams to build make-report parameters
  • Optional parameter idempotenceKey for report-make requests

Changed

  • Method userReportMake in ClientInterface (now he takes ReportMakeParams as parameter)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I wrote unit tests for my code (if tests is required for my changes)
  • I have made changes in CHANGELOG.md file

@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #17 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              master       #17   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       461       479   +18     
===========================================
  Files             33        34    +1     
  Lines           1137      1180   +43     
===========================================
+ Hits            1137      1180   +43     
Flag Coverage Δ Complexity Δ
#php 100.00% <100.00%> (ø) 479.00 <28.00> (+18.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
src/Client.php 100.00% <100.00%> (ø) 45.00 <6.00> (-4.00)
src/Params/ReportMakeParams.php 100.00% <100.00%> (ø) 22.00 <22.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 c54c935...46fe77f. Read the comment docs.

@tarampampam
Copy link
Contributor

Can you write tests for ReportMakeParams class too?

@gomzyakov
Copy link
Contributor Author

@tarampampam

Can you write tests for ReportMakeParams class too?

Yes, of course. Since your review, I:

  • Rename ReportMakeParams to ReportMakeRequest (
  • Wrote tests for ex-ReportMakeParams
  • Move some logic to ReportMakeRequest::getBodyObject method

Its OK?

Notice: I forgot to put the "Draft" tag in the title of the PR. An hour ago, the PR was unfinished, sorry)

@tarampampam
Copy link
Contributor

tarampampam commented Oct 9, 2020

Looks good! But:

  • ReportMakeRequest your class is not request. It seems like parameters, so, rename it back, please
  • Do not keep logic in parameters wrapper - this is a bad practice.

And last - documentation. Code samples (in readme file, for example) must be updatated too

@tarampampam tarampampam marked this pull request as draft October 9, 2020 05:41
@gomzyakov gomzyakov marked this pull request as ready for review October 9, 2020 08:59
@gomzyakov
Copy link
Contributor Author

Looks good! But:

  • ReportMakeRequest your class is not request. It seems like parameters, so, rename it back, please
  • Do not keep logic in parameters wrapper - this is a bad practice.

And last - documentation. Code samples (in readme file, for example) must be updatated too

All comments considered

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Params/ReportMakeParams.php Outdated Show resolved Hide resolved
src/Params/ReportMakeParams.php Outdated Show resolved Hide resolved
src/Client.php Outdated Show resolved Hide resolved
src/ClientInterface.php Outdated Show resolved Hide resolved
src/Params/ReportMakeParams.php Outdated Show resolved Hide resolved
src/Params/ReportMakeParams.php Outdated Show resolved Hide resolved
src/Params/ReportMakeParams.php Outdated Show resolved Hide resolved
src/Params/ReportMakeParams.php Outdated Show resolved Hide resolved
@avtocod avtocod deleted a comment from gomzyakov Oct 12, 2020
@tarampampam tarampampam marked this pull request as draft October 12, 2020 06:28
Copy link
Contributor

@tarampampam tarampampam left a comment

Choose a reason for hiding this comment

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

Please, follow PR comments

@tarampampam tarampampam marked this pull request as ready for review October 12, 2020 08:41
README.md Outdated Show resolved Hide resolved
src/Params/ReportMakeParamsInterface.php Outdated Show resolved Hide resolved
gomzyakov and others added 2 commits October 12, 2020 15:00
Co-authored-by: Paramtamtam <7326800+tarampampam@users.noreply.github.com>
@tarampampam tarampampam added the enhancement New feature or request label Oct 12, 2020
@tarampampam tarampampam self-requested a review October 12, 2020 12:27
@tarampampam
Copy link
Contributor

"Parameter classes finalization" versus "Parameter interfaces with defined getters". Friendly ping for @eldario

@eldario
Copy link
Contributor

eldario commented Oct 13, 2020

@tarampampam @gomzyakov

I don't see the need to use interfaces for the ReportMakeParams. Therefore, I propose to prevent them from being overridden in child classes by adding Final to the class.

@gomzyakov gomzyakov marked this pull request as draft October 13, 2020 06:37
@gomzyakov gomzyakov marked this pull request as ready for review October 13, 2020 06:51
@gomzyakov
Copy link
Contributor Author

@tarampampam I ran the feature-tests again. All OK.

@tarampampam
Copy link
Contributor

👍 Great thanks for your contribution!

@tarampampam tarampampam merged commit 31a3aa2 into avtocod:master Oct 13, 2020
@tarampampam tarampampam added this to the v4.x milestone Oct 13, 2020
@tarampampam tarampampam linked an issue Oct 13, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal to change the method userReportMake
4 participants