Skip to content

Conversation

@atilaneves
Copy link
Contributor

Reverts #7886

@atilaneves atilaneves requested a review from pbackus as a code owner March 26, 2021 18:57
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @atilaneves! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21731 enhancement SumType should provide convenient access to the type index

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7922"

Copy link
Member

@n8sh n8sh left a comment

Choose a reason for hiding this comment

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

Approving and marking for immediate merging since the submitter is the one who has to approve all new public symbols and he does not appear to have approved the PR being reverted.

@dlang-bot dlang-bot merged commit 7407ce9 into master Mar 26, 2021
@ibuclaw ibuclaw deleted the revert-7886-sumtype-type-index branch March 26, 2021 21:32
@Geod24
Copy link
Member

Geod24 commented Mar 27, 2021

Approving and marking for immediate merging since the submitter is the one who has to approve all new public symbols and he does not appear to have approved the PR being reverted.

It would have been nice to have an explanation as to why this is being reverted though. Any kind of context you might have right now will not be known to other people 6 months from now.

@atilaneves
Copy link
Contributor Author

Approving and marking for immediate merging since the submitter is the one who has to approve all new public symbols and he does not appear to have approved the PR being reverted.

It would have been nice to have an explanation as to why this is being reverted though. Any kind of context you might have right now will not be known to other people 6 months from now.

The rationale is that APIs should prevent misuse, and not going through match is IMHO not the way to use a sum type. All cases should be handled, every time.

@pbackus
Copy link
Contributor

pbackus commented Mar 29, 2021

The rationale is that APIs should prevent misuse, and not going through match is IMHO not the way to use a sum type. All cases should be handled, every time.

I agree with this, but do not understand how it justifies excluding typeIndex, sincetypeIndex does not allow the user to bypass match or avoid handling all cases. Is some other kind of misuse you believe it would enable?

@atilaneves
Copy link
Contributor Author

The rationale is that APIs should prevent misuse, and not going through match is IMHO not the way to use a sum type. All cases should be handled, every time.

I agree with this, but do not understand how it justifies excluding typeIndex, sincetypeIndex does not allow the user to bypass match or avoid handling all cases. Is some other kind of misuse you believe it would enable?

Good point. I still don't see why this would be useful though, and after asking in the forum I didn't see any compelling answers.

@pbackus
Copy link
Contributor

pbackus commented Mar 30, 2021

Good point. I still don't see why this would be useful though, and after asking in the forum I didn't see any compelling answers.

The main answer given on the forums was serialization. For example, having access to typeIndex makes it easy to serialize

SumType!(int, string)("hello")

to

{ "type": 1, "value": "hello" }

Of course, you could equally well choose a different schema for your serialized data; for example

  1. { "type": "string", "value": "hello" }; or even just
  2. "hello"

...but these have their own disadvantages. (1) requires you to define a mapping between types and strings, which is non-trivial in the general case, and (2) makes it impossible to distinguish a plain string from a SumType in the serialized data. I wouldn't go so far as to say that the typeIndex approach is the best one, but there are legitimate reasons one might prefer it over the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants