Skip to content

Conversation

Maarty
Copy link
Contributor

@Maarty Maarty commented Apr 7, 2025

Those methods were missing in the interface. Added so they can be easily mocked in tests.

@drieseng
Copy link
Member

drieseng commented Apr 7, 2025

@Rob-Hague, shouldn't we be consistent with the presence/absence of a default value for the cancellationToken argument? Personally, I prefer not to have a default value.

@Maarty
Copy link
Contributor Author

Maarty commented Apr 7, 2025

@drieseng, I agree, it should be united, I can edit it in all the methods in this PR. Currently, it is half/half - presence/absence.

However, isn't cancellationToken with a default value more standard way? It is very common to have a default value in the .NET APIs, also they somehow recommend it in their blogs - Recommended patterns for CancellationToken.

Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

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

My preference is to minimise breaking changes i.e. keep it as it is. Adding to an interface is already a breaking change but it seems that most people are just using it for mocking and similar has been done in previous releases, so OK

Thanks for the PR

@Rob-Hague Rob-Hague merged commit 1cc527e into sshnet:develop Apr 10, 2025
4 checks passed
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.

3 participants