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

WIP nested each with equal values #1686 #1687

Draft
wants to merge 2 commits into
base: 4.x
Choose a base branch
from

Conversation

graphicore
Copy link

@graphicore graphicore commented May 5, 2020

Before creating a pull-request, please check https://github.com/wycats/handlebars.js/blob/master/CONTRIBUTING.md first.

Generally we like to see pull requests that

  • Please don't start pull requests for security issues. Instead, file a report at https://www.npmjs.com/advisories/report?package=handlebars
  • Maintain the existing code style
  • Are focused on a single change (i.e. avoid large refactoring or style adjustments in untouched code if not the primary goal of the pull request)
  • Have good commit messages
  • Have tests
  • Have the typings (lib/handlebars.d.ts) updated on every API change. If you need help, updating those, please mention that in the PR description.
  • Don't significantly decrease the current code coverage (see coverage/lcov-report/index.html)
  • Currently, the 4.x-branch contains the latest version. Please target that branch in the PR.

@graphicore
Copy link
Author

graphicore commented May 5, 2020

Note, the proposed fix in 234cf2f is just a suggestion. I'm really not deep enough into the code base to judge if the proposed fix is doing any harm. I put an explanation into the commit message:

Add explicit goDeeper option to wraped program;Fixes #1686

According to the commit message of 279e038:

" [...] Helper authors now need to take care to return the same context
value whenever it is conceptually the same and to avoid behaviors that
may execute children under the current context in some situations and
under different contexts under other situations."

I belive that metric is too implicit, as the issue demonstrates. This
is because in this case the context value is not conceptually the same,
but it is de facto the same value.

Instead this suggests to use an explicit option flag "goDeeper" to mark
when a helper definitely should go deeper. This also should be backwards
safe, as it doesn't change the behavior of existing helpers, builtin or
custom, except the builtin "each",

Move test case into parent test suite "regressions".
…1686

According to the commit message of 279e038:

" [...] Helper authors now need to take care to return the same context
value whenever it is conceptually the same and to avoid behaviors that
may execute children under the current context in some situations and
under different contexts under other situations."

I belive that metric is too implicit, as the issue demonstrates. This
is because in this case the context value is not conceptually the same,
but it is de facto the same value.

Instead this suggests to use an explicit option flag "goDeeper" to mark
when a helper definitely should go deeper. This also should be backwards
safe, as it doesn't change the behavior of existing helpers, builtin or
custom, except the builtin "each",
@graphicore graphicore changed the title Add failing unit test for #1686 WIP Add failing unit test for #1686 May 5, 2020
@graphicore graphicore changed the title WIP Add failing unit test for #1686 WIP nested each with equal vales #1686 May 5, 2020
@graphicore graphicore changed the title WIP nested each with equal vales #1686 WIP nested each with equal values #1686 May 5, 2020
@jaylinski jaylinski added the bug label Nov 23, 2021
@jaylinski jaylinski marked this pull request as draft September 6, 2023 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants