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

Adding a param to customize the access modifier (supersedes #81) #84

Merged
merged 8 commits into from
Nov 1, 2017

Conversation

AliSoftware
Copy link
Collaborator

@AliSoftware AliSoftware commented Oct 22, 2017

Closes #81

This is a follow-up of #81 which seemed to be a little bit abandoned by @txaiwieser

I used a little different take (just using --param public, as there's no practical use to make the constants something other than public or internal — generating private or file private constants would be useless) hence the separate PR.

  • CHANGELOG
  • Add tests which change that parameter when generating the output, to ensure it's taken into account on every declarations that matters

@djbe
Copy link
Member

djbe commented Oct 22, 2017

Just nitpicking:
Is public clear enough as a parameter? Can it cause conflicts at some future point?

@AliSoftware
Copy link
Collaborator Author

Good point, what about --param publicAccess?

@djbe
Copy link
Member

djbe commented Oct 22, 2017

Yeah, sounds good.

@AliSoftware
Copy link
Collaborator Author

AliSoftware commented Oct 26, 2017

Once #85 and #87 are merged we'll have to be careful to add the {{accessModifier}} to the lines changed in #85/#87 into this PR, so that the getter for imageName/colorName are affected by the internal/public param too

@AliSoftware AliSoftware force-pushed the feature/access-modifier branch from c68afdd to 811b13b Compare October 26, 2017 09:59
@AliSoftware AliSoftware force-pushed the feature/access-modifier branch from 811b13b to 55c47eb Compare November 1, 2017 17:47
@djbe
Copy link
Member

djbe commented Nov 1, 2017

Unrelated to this PR (but noticed it while reviewing):

We still generate all the variations for swift 2, even though we don't compile these afterwards. Is it worth it keeping those around, and still generating them? They're marked as deprecated in the documentation.

test(template: "swift2",

It might be worth it, for unsupported swift versions, to not generate the variations and only have the default output.

@AliSoftware
Copy link
Collaborator Author

Tbh in even wonder if it's worth keeping Swift 2 templates at all…

@@ -2,19 +2,20 @@

{% if palettes %}
{% set enumName %}{{param.enumName|default:"ColorName"}}{% endset %}
{% set accessModifier %}{% if param.publicAccess %}public{% else %}internal{% endif %}{% endset %}
Copy link
Member

@djbe djbe Nov 1, 2017

Choose a reason for hiding this comment

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

Note: although it's not necessary to mark these as internal per se, I think it's good to actually do it, as it clearly communicates this to the user/developer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that's why I took that approach instead of the one in #81 where it was either public when on… and empty string when off.
I prefer the current approach both for balance in the template (as including the space in one case and nothing in the other and not having the space when using {{accessModifier}}func feels strange) and because making internal explicit feels more… well… explicit 😄

Copy link
Member

@djbe djbe left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@djbe
Copy link
Member

djbe commented Nov 1, 2017

Up to Xcode 8, you can use swift 2.3.

I don't think anyone still uses it, and if they still do, they really should migrate. And if they really want, they can always use a custom template based on the current ones. But I vote remove them 👍 (separate PR though)

@AliSoftware
Copy link
Collaborator Author

Yeah that was my thought too. They can always use the templates as custom templates after making a copy of ours from the commit before we remove them 😜
Will likely remove in next version or during the big merge 😉

@AliSoftware AliSoftware merged commit 901f661 into master Nov 1, 2017
@AliSoftware AliSoftware deleted the feature/access-modifier branch November 1, 2017 23:45
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