Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Strings: multiple tables support #49

Merged
merged 6 commits into from
May 28, 2017
Merged

Conversation

djbe
Copy link
Member

@djbe djbe commented May 22, 2017

This adapts both the structured and flat templates to use the table information correctly. The templates will only generate a subenum if there's more than one table.

I've also adapted the flat template to use static constants instead of cases, as discussed during one of the slack conversations. Can I leave it in this PR, or should I split that change off into it's own PR?

Refs SwiftGen/SwiftGenKit#41

@djbe djbe added this to the 2.0.0 milestone May 22, 2017
@djbe djbe changed the title Fixture changes due to multiple tables support Strings: multiple tables support May 22, 2017
@djbe djbe removed the status: WIP label May 22, 2017
@djbe djbe force-pushed the feature/strings-multiple-tables branch from 42ea6f8 to 24a12d7 Compare May 26, 2017 16:10
Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

LGTM for the multiple tables support

Just a quick question: wondering if the fixtures names are still consistent, looking at them? .g. I see some …-defaults-no-comments well if it's --param no-comments then it's not defaults, is it?

@AliSoftware AliSoftware force-pushed the feature/strings-multiple-tables branch from 24a12d7 to e610d2f Compare May 27, 2017 18:47
@AliSoftware
Copy link
Collaborator

@djbe Rebased but with conflicts so I'll let you double-check before merging

@djbe djbe merged commit ca34235 into master May 28, 2017
@djbe djbe deleted the feature/strings-multiple-tables branch May 28, 2017 21:09
@AliSoftware
Copy link
Collaborator

Didn't we forget the CHANGELOG entry again?

@djbe
Copy link
Member Author

djbe commented May 28, 2017

I did ☹️

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

Successfully merging this pull request may close these issues.

2 participants