Skip to content
This repository was archived by the owner on Nov 6, 2019. It is now read-only.

Handle union of string values #96

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

epabst
Copy link
Contributor

@epabst epabst commented Sep 18, 2018

@epabst
Copy link
Contributor Author

epabst commented Nov 13, 2018

Rebased and updated based on feedback.

@Schahen
Copy link
Contributor

Schahen commented Nov 29, 2018

@epabst it looks like comments meta-information is broken in
testData/typeAlias/typeParams.d.kt

Can you please verify on your side as well?
screen shot 2018-11-29 at 2 22 51 pm

@epabst
Copy link
Contributor Author

epabst commented Nov 29, 2018

Great catch! What do you think it should look like since nesting the comments won't compile?

@Schahen
Copy link
Contributor

Schahen commented Nov 29, 2018

Let's discuss. It's seems to me following options are viable.

  • Just leave String | List<String | Number> because actually the only way those comments are helpful now is as hints - and this is a perfectly valid hint.
  • Omit String | Number part completely.

First option looks nicer to me)

This fixes losing the nullability indicator.
@epabst
Copy link
Contributor Author

epabst commented Dec 27, 2018

@Schahen I've fixed the issue with losing the comments and improved them as you've suggested. The fix initially broke nullability, which I then fixed, including adding more tests. The fix had a side-effect of preserving the original order of unioned types. Please re-review.

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

Successfully merging this pull request may close these issues.

2 participants