Skip to content

revert union changes#45

Merged
ferozco merged 5 commits into
developfrom
fo/revert-union-changes
Jul 18, 2018
Merged

revert union changes#45
ferozco merged 5 commits into
developfrom
fo/revert-union-changes

Conversation

@ferozco
Copy link
Copy Markdown
Contributor

@ferozco ferozco commented Jul 18, 2018

Revert #22. Since it was originally just an experiment and it caused #44, I hope its ok. I am keen to open up the discussion on how implement unions, especially if we have some numbers around the performance of one approach vs. another

@ferozco ferozco requested review from iamdanfox and markelliot July 18, 2018 14:59
Copy link
Copy Markdown
Contributor

@markelliot markelliot left a comment

Choose a reason for hiding this comment

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

👍


@Value.Derived
default String getterName() {
return asGetterName(fieldName().get());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With this refactor, asGetterName is now dead code

@@ -160,3 +160,6 @@ types:
union:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need this Union type at all, given that there is also a UnionTypeExample type that does a similar thing but with more variants?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: it's not used anywhere

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left it there so we can have more cases covered by tests

@ferozco ferozco merged commit 882333c into develop Jul 18, 2018
@iamdanfox iamdanfox deleted the fo/revert-union-changes branch July 18, 2018 19:01
carterkozak pushed a commit to carterkozak/conjure-java that referenced this pull request Dec 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants