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

Fix issue 32. Reorganized Service class to provide clients in props #38

Merged

Conversation

vitali-sonchyk-epam
Copy link
Contributor

No description provided.

@vitali-sonchyk-epam vitali-sonchyk-epam changed the title Fix issue 32 Fix issue 32. Reorganized Service class to provide clients in props Mar 3, 2019
@codecov-io
Copy link

codecov-io commented Mar 4, 2019

Codecov Report

Merging #38 into releases/3.0.0 will decrease coverage by 0.4%.
The diff coverage is 96.36%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           releases/3.0.0      #38      +/-   ##
==================================================
- Coverage           95.12%   94.72%   -0.41%     
==================================================
  Files                  41       42       +1     
  Lines                 615      644      +29     
==================================================
+ Hits                  585      610      +25     
- Misses                 30       34       +4
Impacted Files Coverage Δ
...ient/Api/TestItem/Request/FinishTestItemRequest.cs 100% <ø> (ø)
...Portal.Client/Api/Log/Request/AddLogItemRequest.cs 100% <ø> (ø)
...pi/TestItem/Request/AssignTestItemIssuesRequest.cs 100% <ø> (ø)
ReportPortal.Client/Converter/StringTrimmer.cs 100% <ø> (ø)
ReportPortal.Client/Extention/UriExtensions.cs 100% <ø> (ø)
....Client/Api/Launch/Request/AnalyzeLaunchRequest.cs 100% <ø> (ø)
....Client/Api/Launch/Request/MergeLaunchesRequest.cs 100% <ø> (ø)
...lient/Api/TestItem/Request/StartTestItemRequest.cs 100% <ø> (ø)
...nt/RetryWithExponentialBackoffHttpClientHandler.cs 93.47% <ø> (ø) ⬆️
ReportPortal.Client/Converter/EnumConverter.cs 100% <ø> (ø)
... and 40 more

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 e3a7758...39dd991. Read the comment docs.

@nvborisenko
Copy link
Member

@aliaksandrbasau Alex, your opinion is strongly appreciated.

What is better:

  • service.LaunchClient.GetLaunchAsync(id)
  • service.Launch.GetAsync(id)

This PR is almost about restructuring classes to split them by namespaces to be possible easy increase api coverage. Since we are breaking everything here, don't hesitate to break everything.

We need design client library very well to avoid breaking changes in future. One more thing I keep in mind - api versioning. What if server will support /api/v2 endpoint, how we are going to support it.

@abasau
Copy link

abasau commented Mar 4, 2019

@nvborisenko I wouldn't use Client in class and property names as those classes map to REST collections (or resources). I will give it more thoughts later in the evening and give you more detailed answer.

@abasau
Copy link

abasau commented Mar 5, 2019

@nvborisenko
Actually Client may also work (at least for class names) if those clients are intended to be used separately from the Service class. They map to what Report Portal API documentation calls as controllers. So they can be some kind of ControllerClients.
And I would rename Service class to Client or ServiceClient to be more precise about the purpose of the class.

Another way to answer the question is to check how other projects call similar types of classes. E.g. Swagger CodeGen that can generate c# client from Swagger file. It generates one ApiClient class (that is responsible for serialization, deserialization, making requests) and bunch of Api classes (one per controller) that consume ApiClient and talk to controllers via ApiClient.
image

As for LaunchClient vs Launch, I would go with Launch because almost all properties under Service as clients.

As for GetLaunchAsync vs GetAsync, GetLaunchAsync might be a better choice since the name makes it clear what it "gets".

To sum up:
class LaunchClient => class LaunchControllerClient
class Service => ServiceClient or ReportPortalServiceClient
Service.LaunchClient => Service.Launch
GetLaunchAsync

@vitali-sonchyk-epam
Copy link
Contributor Author

vitali-sonchyk-epam commented Mar 5, 2019

@aliaksandrbasau , do u have any suggestions regarding the api versioning?
currently the api prefix is hardcoded in Service constructor.

@abasau
Copy link

abasau commented Mar 5, 2019

@vitali-sonchyk-epam Yep, forgot to comment on versioning.
Currently version prefix is storied in ReportPortal.config.json (e.g. https://rp.epam.com/api/v1/).
If the prefix is moved into the client (hard-coded like BaseUri.Append($"v1/{Project}/launch")), the client would have full control over what endpoints to use and end-users would make less mistakes by specifying wrong API version (as long as they know not to put version into API url in the config file). And we would be able to use different version of API for different requests (if needed) and we could migrate the client to a newer version of API without any effort on end-users' side.
The only downsides of the approach are that the compatibility is broken and that end-users have to be notified about the changes in URL format.

@nvborisenko nvborisenko merged commit 2ee2f7c into reportportal:releases/3.0.0 Mar 6, 2019
@nvborisenko
Copy link
Member

Thanks all, further work is coming separately.

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

5 participants