C#: Added Duration deconstruction capability.

[mwpowellhtx]

Pursuant to the issue.

Thank you.

[mwpowellhtx]

@Mizux Any ideas?

Please include release notes: yes AND Please include at least a language label (e.g., c++, java, python). Or apply one of the following labels: bazel, cmake, cleanup, conformance tests, integration, protoc.) OR Please include release notes: no

Release notes not required?

AFAIK, I included the C# language label, however. That is case sensitive, perhaps?

[mwpowellhtx]

@Mizux AFAIK, could be falling over on this:

{"must_include":{"regex":"^(c#|c\\+\\+|cleanup|conformance tests|integration|java|javascript|go|objective-c|php|python|ruby|bazel|cmake|protoc)","message":"Please include at least a language label (e.g., c++, java, python). Or apply one of the following labels: bazel, cmake, cleanup, conformance tests, integration, protoc."}}]}

I am not 100% positive, c# should not be c\#, and/or allow the full name, i.e. csharp.

Why else would it be falling over?

[mwpowellhtx]

@Mizux Would like to put this to bed and move on. Do you happen to know of a protobuf POC I can mention here?

[mwpowellhtx]

Anyone at all? Why is Mergeable breaking? The current theory is that a commit comment needs to be addressed. Assuming that is the case, did so, i.e. C#: Added Duration deconstruction capability.. Is still falling over there. Is it a regex issue? c# needs to be c\\#, perhaps, for instance? Or even allow for csharp? Or dotnet for that matter.

[jskeet]

@anandolee please could you have a look at the CI check failure?

[jskeet]

Drive-by review comment (not a full review): I would definitely not use int? for the nanoseconds parameter. If the value is 0, that doesn't indicate that it wasn't specified or should be ignored in some way. A value of "5 seconds and 0 nanoseconds" is precisely that, it's not "5 seconds and some indeterminate number of nanoseconds".

[mwpowellhtx]

@jskeet Appreciate the review; the intent there was "unspecified" not "indeterminate". Which is precisely what a Nullable<T> null is. In keeping with the minimum requirement when instantiating a Duration, i.e.

new Duration { Seconds = seconds }

Or,

new Duration { Seconds = seconds, Nanos = nanoseconds }

Or for that matter,

new Duration { Nanos = nanoseconds }

I suppose the latter would even be possible.

Edit: At least considering the usage discovered via Google.OrTools, for example.

[jskeet]

But the point is that you don't know it's unspecified. You can't tell the difference between new Duration { Seconds = seconds } and new Duration { Seconds = seconds, Nanos = 0 } so why pretend that there is a meaningful difference? All that happens is 0 becomes an impossible value to return from the Deconstruct method, even if it was explicitly specified. I don't think that's a good idea - and it's inconsistent with just fetching the Nanos property directly.

Perhaps you should indicate what benefit you believe is derived from this inconsistency. (Note that if the designers of the Duration message had wished there to be a meaningful distinction between 0 and "not specified", we'd have used Int32Value instead of just int32, at which point the property would have been of type int?, and then it would make sense to deconstruct it to int? as well.)

[mwpowellhtx]

Okay so the assumption is that "unspecified" is rather "default" 0. Fair enough. I am not attached to the int? notion by any means.

Establishing that as the assumption, then, yes, of course, it would make better sense to decon to the actual type, i.e.

void Deconstruct(out long seconds, out int nanoseconds) { ... }

[mwpowellhtx]

Cleaned up branch forthcoming. In the meantime, are we talking about also adding a note in the CHANGES.txt file?

[Mizux]

looking at https://github.com/protocolbuffers/protobuf/blob/master/.github/mergeable.yml I would try to add "release notes: yes" in the body of the commit message, my guess the CHANGE.txt is autogenerated using commit message "tags" etc...

[jskeet]

@mwpowellhtx: Is this PR still required? It's now over two years since the last comment, and the code still has int? instead of int (and no tests).

I'm happy to help review and get it in if you really think it's needed, but personally I'd recommend just living without it if it hasn't been too painful for the last two years.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.

This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.