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

Make CancellationToken optional from tests #2877

Closed
wants to merge 1 commit into from

Conversation

kzu
Copy link

@kzu kzu commented Feb 7, 2019

Since CancellationToken.None is equivalent to default(CancellationToken)
according to https://github.com/dotnet/corefx/issues/495#issuecomment-72322134,
and since the latter can further be shortened to just default, make use of
both to cleanup a bunch of method calls that all ended up using just
CancellationToken.None.

Since `CancellationToken.None` is equivalent to `default(CancellationToken)`
according to https://github.com/dotnet/corefx/issues/495#issuecomment-72322134,
and since the latter can further be shortened to just `default`, make use of
both to cleanup a bunch of method calls that all ended up using just
`CancellationToken.None`.
@sharwell
Copy link
Member

sharwell commented Feb 7, 2019

This is currently a style matter for this repository, along with the use of ConfigureAwait(false) in tests. It's come up a few times before, e.g. #518, #1440. It's mostly about the desire to eventually have xunit/xunit#345 so user cancellation is a test input that's always available.

@sharwell
Copy link
Member

sharwell commented Feb 7, 2019

It's possible this style changes in the future, but at this time I'm not ready to apply it. If it changes, the cancellation token would be omitted from signatures altogether as opposed to making it optional.

@sharwell sharwell closed this Feb 7, 2019
@sharwell sharwell added wontfix and removed blocked labels Feb 7, 2019
@kzu
Copy link
Author

kzu commented Feb 7, 2019

Hm... the reason for not wanting to make it optional, however, has nothing to do with what I submitted here, since the optional cancellation made optional is not in any test-defining method.

@kzu
Copy link
Author

kzu commented Feb 7, 2019

I know this is just a matter of taste, but lines were already long enough that I was scrolling just to find out that it was all just CancellationToken.None and ConfigureAwait(false). Makes the code less readable and (at least in the former case) it's not needed at all. Not sure about the latter in the context of a unit test either.... My guess is it's not needed there either, but I may be wrong...

@kzu
Copy link
Author

kzu commented Feb 7, 2019

In any case, getting a PR rejected in 3' after all that regex-fu, is a bit discouraging... Anyway, on to what I was really doing now... ¯\(ツ)

@sharwell
Copy link
Member

sharwell commented Feb 7, 2019

In any case, getting a PR rejected in 3' after all that regex-fu, is a bit discouraging.

Yeah, I knew that was going to be the case, but I figured it was better to close it early than leave it sitting here to never go anywhere. ☹️

@kzu
Copy link
Author

kzu commented Feb 7, 2019

What about your comment that you didn't want it because of the issue with discovering/instantiating/invoking/whatever unit tests when optional parameters where part of the signature? Which is clearly different to what this PR does?

The xunit/xunit#345 issue isn't implemented and even if it were, this PR doesn't prevent a specific test to provide a cancellation if needed/wanted, there's just a default in case you don't care, which is currently the case in all the removed code. So, it just removes all the useless boilerplate all over that's currently not adding any value... So I fail to see how it limits any such future feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants