-
Notifications
You must be signed in to change notification settings - Fork 225
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
Added the option to throw when variable not found in context #289
base: master
Are you sure you want to change the base?
Conversation
6f6a983
to
1739076
Compare
@kylef @djbe @AliSoftware Could you have a look? |
let arguments = try filter.1.map { try $0.resolve(context) } | ||
return try filter.0.invoke(value: value, arguments: arguments, context: context) | ||
} | ||
|
||
if filteredResult == nil, context.environment.throwOnUnresolvedVariable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel like this is a correct place for this check. nil can be a result of filter operation itself. Also it does not cover usage of undefined variables outside of filter.
What we need to do is to check for undefined variable in Variable.resolve
method when we do if current == nil { return nil }
and to throw instead of returning nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! :) I will break it down into some points:
a) "nil can be a result of filter operation itself": yes, that is correct. It is intentional, this is the reason why I implemented this feature: the main idea behind it is that getting a nil in a template indicates a bug (either because the variable is not defined or because of a filter failing to find a matching result), and instead of silently not printing anything, I would prefer to throw.
b) "Also it does not cover usage of undefined variables outside of filter": I do not fully understand what you mean, seems to be working correctly as per the test testUnresolvedVariable
I added. I am not very familiar with the Stencil codebase, can you give me an example of template where this will not work?
c) "What we need to do is to check for undefined variable in Variable.resolve method": I also tried that, but then the default
filter will be useless, as we will be throwing even when a default value is provided.
I added some more tests to show how I expect this feature to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of throwOnUnresolvedVariable
I should call the option throwOnUnresolvedExpression
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ilyapuchka could you have a look a my latest comments and give me some feedback?
… found in the context
Related to #248 |
What'd be nice is if we could have a
Although it may even be an OptionSet, so that users can configure these things individually (nil access, non-existing access). That way, at a later point we may define extra behaviours such as KVO selectors, etc... |
For the name of such an enum I propose |
@djbe Can you put some usecase examples? The second one you mention I do not see the usecase:
|
Also, in the
|
Right the |
I can look into it, but, is there any possibility of getting it merge? Just asking because I see many unmerged PRs in this repo. Also, are you okey with the current implementation for EDIT: also please see my comment about the problems I find with the And:
Thanks for answering btw :) |
The existing behaviour substitutes a
variable that could not be resolved
for an empty string without throwing any error.This PR adds the option to throw when a
variable that could not be resolved
is used in a template (which means it was not defined in the context).This is helpful to prevent errors in templates, for example if introducing a typo when referring to a variable.