Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to inject custom parameters to the Stencil context via command line #253

Closed
AliSoftware opened this issue Jan 3, 2017 · 18 comments
Closed

Comments

@AliSoftware
Copy link
Collaborator

AliSoftware commented Jan 3, 2017

One thing that would add a lot of flexibility in SwiftGen is to be able to inject any custom parameters to the template from the command line:

swiftgen strings Localizable.strings --param MyParam Value1 --param OtherParam Value2

This would allow us to generalise the special case of --enumName X: the --enumName parameter would disappear in profit of the new --param enumName X way of injecting it.

For example one could imagine using --param AppModule MyAwesomeApp to inject a custom value for the AppModule variable in the Stencil context. Then in the storyboard templates, we'd be able to use {% if AppModule %}{{AppModule}}.{% endif %} to prefix all class names with the app's module name (to allow disambiguation) if its value has been provided via the command line.

@krzysztofzablocki
Copy link
Collaborator

arguments.AppModule for consistency with Sourcery and avoiding name clash

@AliSoftware
Copy link
Collaborator Author

@krzysztofzablocki Does Sourcery also named its CLI flag --param?

If so, why not use param.AppModule in the Stencil context for consistency between the CLI flag and the context?

@AliSoftware
Copy link
Collaborator Author

Also, do you do KeyPath parsing?

Like if the user invokes Sourcery with --param foo.bar 1 --param foo.baz 2, does the dictionary created under the "arguments" entry has 2 entries named "foo.bar" and "foo.baz", or do you create an entry "foo" which will itself be a dictionary with "bar" and "baz"?

(I don't think that last solution is worth implementing, but just asking in case you do that, for consistency's sake too)

@krzysztofzablocki
Copy link
Collaborator

krzysztofzablocki commented Jan 5, 2017

Right now our version is very simple, we'll need to figure out a way for parsing those extra args properly with commander, related discussion krzysztofzablocki/Sourcery#80

Whatever we do here best we do it in both tools ;) I prefer your syntax for passing in the extra arguments to CLI but I also think we should scope them under a single context item in Stencil

@AliSoftware
Copy link
Collaborator Author

Yeah I totally agree with your suggestion in scoping them in a single item in the context, hence my question about the name of such key (arguments vs param vs params) 😉

@djbe
Copy link
Member

djbe commented Jan 8, 2017

Why scope them though? It just ends up in more verbose templates. Could be useful if a user wants to override variables provided by the parser?

@krzysztofzablocki
Copy link
Collaborator

well, that is exactly why I want to scope it to avoid potentially overriding one of the things we will add via parser, what use case can you see there?
that logic for composing arguments into typed dictionary sounds cool from engineering perspective but it also sounds a bit overkill for most use-cases, is that something that rails generator do?

@djbe
Copy link
Member

djbe commented Jan 9, 2017

It would be handy if we want to provide defaults but that the user wants to override.

For example EnumName could, by default (provided by the swiftgen parser), have as value ColorName, but the user could override this by passing as CLI argument --param EnumName=MyColors.

@krzysztofzablocki
Copy link
Collaborator

Ok that makes sense, if we do that then lets log info that a param is an override of provided data

@djbe
Copy link
Member

djbe commented Jan 9, 2017

Hmm, interesting suggestion. How would you achieve that? Variables should remain just that for a template.

We could do some additional type-checking if you're worried about that, so that arrays stay arrays, etc...

@krzysztofzablocki
Copy link
Collaborator

krzysztofzablocki commented Jan 9, 2017

well we control how context is defined before feeding templates, so we can just check before assigning incoming value whether it was already there and then log that info.
The reason I think it's useful is because people might make mistakes, just logging that info would be useful at preventing them.

@AliSoftware
Copy link
Collaborator Author

AliSoftware commented Jan 9, 2017

I'm not sure the parser should inject default values anymore.

Stencil provides a default filter, so it would make more sense to me that the template would be the one providing the default value, e.g. {{ param.enumName|default:L10n }} (and the context won't have an enumName entry anymore)

This way, the default value isn't hard-coded in the source code (nor of SwiftGen nor of Sourcery) and we can still override it from the command line using --param enumName=Foo. And then no real need for logging anything I think, as that would be the expected behaviour and what params are designed for anyway.

@krzysztofzablocki
Copy link
Collaborator

krzysztofzablocki commented Jan 9, 2017

👍 especially with set node you could easily express all arguments on top of your templates along their defaults

@AliSoftware
Copy link
Collaborator Author

Besides, by scoping the parameters provided by the command line inside a param key in the context, we make it extra clear in the template that this value can be overridden, unlike some others (you wouldn't want the strings entry to be overridable for example).


And this would even make it possible to write Unit Tests for the documentation, like grep-ing \wparam\. in each .stencil file and ensure there's a corresponding row in the corresponding markdown template documentation listing this overridable parameter (like here in this sample template doc)

@krzysztofzablocki
Copy link
Collaborator

krzysztofzablocki commented Jan 9, 2017

@AliSoftware in case of SwiftGen I'll raise you even better, you could just scan the given template for the param name and fail build if someone passes in unused argument, thus preventing human mistakes / typos

@AliSoftware
Copy link
Collaborator Author

That could be a nice idea, but might be dangerous and prone to false positives. Like imagine this hypothetical example:

{% for c in param.comment %}
// {{ c.text }}
{% endif %}

And calling SwiftGen with:

swiftgen … --param comment.text=Foo --param comment.text=Bar --param comment.text=Baz

It would be hard to see that we access the comment.text parameter in two steps and thus that --param comment.text is valid to pass as an argument…

@AliSoftware
Copy link
Collaborator Author

We can now call enrich(context:) from SwiftGen cli now that SwiftGen/StencilSwiftKit#8 is merged.

This could be nice to integrate that in the upcoming SwiftGen 4.2 — and make use of it in the storyboards templates to get rid of the import warning mentioned in #264 as I think that's the thing that annoys the most people in the current 4.1 version (me included) 😉

@djbe
Copy link
Member

djbe commented Feb 16, 2017

Implemented in #265

@djbe djbe closed this as completed Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants