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

Added the option to throw when variable not found in context #289

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acecilia
Copy link

@acecilia acecilia commented Dec 26, 2019

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.

@acecilia acecilia force-pushed the master branch 2 times, most recently from 6f6a983 to 1739076 Compare December 26, 2019 03:02
@acecilia
Copy link
Author

@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 {
Copy link
Collaborator

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.

Copy link
Author

@acecilia acecilia Jan 12, 2020

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.

Copy link
Author

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?

Copy link
Author

@acecilia acecilia Mar 22, 2020

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?

@acecilia
Copy link
Author

Related to #248

@djbe
Copy link
Contributor

djbe commented May 27, 2020

What'd be nice is if we could have a ResolveMode enum (bad at names), as mentioned in #248:

  • Unchecked: accessing nil is okay, non-existing property is okay (returns nil)
  • Strict: accessing nil is okay, non-existing property throws
  • StrictNoNil: accessing nil throws, non-existing property throws

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...

@AliSoftware
Copy link
Collaborator

For the name of such an enum I propose PropertyFallbackBehavior

@acecilia
Copy link
Author

acecilia commented May 27, 2020

@djbe Can you put some usecase examples? The second one you mention I do not see the usecase:

  • Unchecked: thats the current way.
  • Strict:
    • I do not see the usecase for this: under which scenario will you be accessing nil if a property returned a value correctly? And in that case, why would you like it not throwing?
    • Also, the context is a dictionary. I do not think there is any difference between a key which value is nil and a non existing key. Is there any way to differentiate one or the other?
  • StrictNoNil: the implementation of this PR, where it throws if the final value after applying all the filters is nil.

@acecilia
Copy link
Author

acecilia commented May 27, 2020

Also, in the StrictNoNil (this PR), if I expect something may be nil and I do not want to throw, what I am doing is:

{{ name|default:"" }}

@djbe
Copy link
Contributor

djbe commented May 29, 2020

Right the strict mode I propose above would avoid having to add |default everywhere, but at least throw errors for unknown variables (typos, etc...)

@acecilia
Copy link
Author

acecilia commented May 29, 2020

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 StrictNoNil in this PR? Because @ilyapuchka didnt like it so much.

EDIT: also please see my comment about the problems I find with the strict mode when using it together with filters, bullet point C. I still do not see clearly the usecase, as it kindda makes filters impossible to use.

And:

Also, the context is a dictionary. I do not think there is any difference between a key which value is nil and a non existing key. Is there any way to differentiate one or the other?

Thanks for answering btw :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants