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

Add support for CancellationTokens within client and services methods. #33

Merged
merged 6 commits into from
Oct 5, 2023

Conversation

AhmedZaki99
Copy link
Contributor

This one is a little bigger than usual.

Since HttpClient supports optional CancellationToken in its methods, TodoistRestClient can be changed to accept cancellation tokens and pass them to HttpClient method calls.

First, important things to consider:

  1. Changes to TodoistRestClient should not affect consumers of that class, so the CancellationToken must be optional.
  2. Any changes to the ITodoistRestClient interface would break classes that implement it, so the interface should remain unchanged.

Based on those constraints, to accept the optional CancellationToken, an ICancellableTodoistRestClient interface is added that inherits from ITodoistRestClient and extends it with method overloads that accept the CancellationToken parameter.
TodoistRestClient is then changed to implement ICancellableTodoistRestClient and support cancellation.

Second, cancellation tokens are like async methods; they are “contagious” all the way down the method call chain. This means in order to utilize the usage of cancellation tokens added to TodoistRestClient, then TodoistClient, TodoistTokenlessClient, and all services that depend on them need to have optional CancellationToken parameter in their methods as well.

Changes then were too many so they were split into four commits to become easy to review:

  1. Duplicate documentation comments were replaced by <inheritdoc> tag so they change dynamically when CancellationToken parameters are added to interfaces.
  2. The ICancellableTodoistRestClient is added along with the TodoistRestClient implementation for it.
  3. TodoistClient is updated to support cancellation by checking if the _restClient field implements ICancellableTodoistRestClient and then passes the CancellationToken to it.
  4. All other services that depend on the TodoistClient are updated to accept cancellation tokens and pass them to TodoistClient methods.

I hope you find this contribution useful and not too hard to review.

Classes now utilize the usage of <inheritdoc> to inherit the implemented interface documentation and reduce duplicate documentation comments.
The added interface inherits from ITodoistRestClient and extends it with method overloads that support cancellation.
TodoistRestClient is also changed to implement ICancellableTodoistRestClient.
…lient.

In TodoistClient we typically pass the cancellation token to _restClient if it implements ICancellableTodoistRestClient.
The rest is just passing the cancellation token as a parameter along the method call chain.
…methods.

Cancellation tokens are then passed to the IAdvancedTodoistClient method calls.
@AhmedZaki99
Copy link
Contributor Author

One extra change has been committed:
A constructor overload for TodoistClient has been added that accepts an ICancellableTodoistRestClient argument, to explicitly state that the TodoistClient can be instantiated using a cancellable client implementation.

This change is not necessarily part of the proposed contribution, it's just a design change. It's up to you to include it in the merge if you accept the pull request.

@olsh
Copy link
Owner

olsh commented Sep 29, 2023

Thank you for the contibution 👍🏻
I'll take a look a bit later.

@olsh
Copy link
Owner

olsh commented Sep 30, 2023

Any changes to the ITodoistRestClient interface would break classes that implement it, so the interface should remain unchanged.

I'd choose breaking changes over introducing a new interface in this case.
I suggest that we increase the library major version and add a cancellation token parameter to ITodoistRestClient and delete ICancellableTodoistRestClient.

@AhmedZaki99
Copy link
Contributor Author

Ok, I will add a commit with these changes.

And added cancellation tokens to ITodoistRestClient directly.
@AhmedZaki99
Copy link
Contributor Author

ITodoistRestClient now supports cancellation tokens directly, and implementing classes like Todoist.Net.TodoistRestClient and Todoist.Net.Tests.RateLimitAwareRestClient has been changed accordingly.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@olsh olsh merged commit 7913fd0 into olsh:master Oct 5, 2023
@AhmedZaki99 AhmedZaki99 deleted the patch-2 branch October 5, 2023 12:38
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.

2 participants