Context param for Include block #214
Conversation
|
@ilyapuchka @AliSoftware Could I get a review on this? |
Sources/Include.swift
Outdated
| @@ -25,7 +27,8 @@ class IncludeNode : NodeType { | |||
|
|
|||
| let template = try context.environment.loadTemplate(name: templateName) | |||
|
|
|||
| return try context.push { | |||
| let subContext = includeContext.flatMap{ context[$0] as? [String: Any] } | |||
There was a problem hiding this comment.
Side note: seing that makes me wonder if SwiftLint is enabled on Stencil (Doesn't Swiftlint warn about a missing space before {?), could be worth adding if it isn't.
There was a problem hiding this comment.
You should also update the docs (at least the one in docs/builtins.rst)
Also, idk how you want to operate from now on now that we're all contributors, but on my other OSS projects I like to add the entries in the CHANGELOG during the PR (in a # master top entry pending release), in order not to forget to list each change when times come to do a release, to avoid adding them only at release time by manually reviewing every PR which had been merged since last release to list them all
|
@yonaskolb Also, I'm curious as to why you pushed that on your fork — which should not be necessary anymore now that you're a core contributor and have push access — instead of pushing that branch directly on the original repo? I'm guessing it's because it's a branch that were already open in your end before you became one of the co-owners (actually you don't seem to have accepted my invitation to be part of the team?), but that would still give you some more power (add reviewers, etc), so 😉 Usually in most projects, CI is disabled for forks for security reasons (would otherwise allow some ill-intended user to expose tokens otherwise, iirc), so pushing a branch on the main repo rather than the fork is generally preferred when possible also for that reason (even if idk if that's the case for Stencil, would have to check on travis). |
|
Thanks @AliSoftware, comments addressed. Yeah if no one person is managing releases we should add changelog entries in PRs 👍 Yes, the branch is just an older one from my fork. |
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Master | |||
|
|
|||
| ### Enhancements | |||
|
|
|||
| - added an optional second parameter to the `include` tag for passing a sub context to the included file | |||
There was a problem hiding this comment.
Maybe we should directly start applying the new changelog format? End with a . (dot + 2 spaces), and the author + issue link. See https://github.com/SwiftGen/SwiftGen/blob/master/CHANGELOG.md for examples.
Nitpick: start sentences with an uppercase.
This is a redo of #95 which only includes the context param which gets passed to the file.
I've also removed the
usingfrom the param