Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[API Implementation]: Use TimeSpan everywhere we use an int for seconds, milliseconds, and timeouts (Group 1/3) #64860
[API Implementation]: Use TimeSpan everywhere we use an int for seconds, milliseconds, and timeouts (Group 1/3) #64860
Changes from 48 commits
377d39c
4d5b632
3477de9
ca4e786
98664c6
94412fd
31b2210
a3d856b
77e959b
ea86b67
517b8e7
47cb9f8
829d412
f413126
516041c
74fb1d9
7deaa98
6827e20
615b66e
6413304
004914c
091c289
70c4efa
eeeb61b
7446821
5fba0ab
738b9e2
e6d68f9
4a35788
486a761
0a0794f
9ffe1ae
7d4af1d
27fa0ce
abea06f
340e5eb
0ff42de
e61acf0
5279de8
aad5405
1529601
c3ea515
53bed23
80f10be
6805ad4
f5e4844
cf68f4d
ff9c0e6
3be4f23
099aacb
d659806
eef5d71
b9aca93
ca7edd2
3a47e9d
a469722
6251c56
a888f6e
c6a6c05
cfba2b9
75b8554
03aa02d
25484a4
4c06f93
ea03c06
4d2e023
bba4bd3
c774885
2f47067
0d0015f
77f921f
a97f7d3
7cdf76c
4a73007
f56905d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a setter. Seems like we should have one as it wouldn't make sense to only support getting as
TimeSpan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is so wanted that there is no setter (I have to adapt the documentation of course)
@terrajobst Can you verify that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have adjusted this despite the fact that we are waiting for the answer from Immo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@terrajobst What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartonjs with your API review hat on should we add a setter (was not in approved shape) for symmetry with the int property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's contention then the best answer is probably to bring it up as a separate item to API Review, so we can discuss it in the larger group (if we want to keep it in this PR, then a blocking issue so it floats to the top).
My initial thought was "no", remembering the guideline as "avoid inter-dependent properties"; but it turns out that's just my rephrasing of one nuance of the actual guideline:
With the Guideline As Written, it wouldn't be bad, just that the last writer wins. So, it's probably goodness to have... but since the original proposal didn't have it, and the review discussion didn't add it... it's probably worth having a chat again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deeprobin Let's leave it out for now to get this PR done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading again.... this is an attribute. The 99.99% use case for setting properties on attributes is via
[UsingAnAttribute(..., Property = Value)]
. Since there is no attribute-literal syntax for a TimeSpan, the setter doesn't have much value.Get-only seems more appropriate to me in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to just do what is good enough for Task today:
runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs
Lines 2642 to 2646 in 7db092a
that will be half the strings, and less duplicated code.
if it's important to tell them what's wrong, you could include the value in the string. (Task doesn't bother)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the setter because we don't need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, but you're still repeating this validation code. my suggestion is to replace it everywhere with just the 5 lines above, effectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then you don't need new strings (or just 1 if you choose to include the value, which is nice)