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

Prevents indent on blank lines of partials. #1518

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

Conversation

bdiz
Copy link

@bdiz bdiz commented Mar 29, 2019

This pull request is to prevent indent in partials that have blank lines. It is needed when handlebars is used as a code generator and the spacing of the output is important or linted.

Here is the result of the test in the pull request before the fix:
image

The blank line in Yehuda's function has indent. This is not desirable. After the fix, this line is no longer indented.

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

@nknapp
Copy link
Collaborator

nknapp commented Mar 30, 2019

Sounds like a breaking change to me, and a pretty special use-case. It should at least be a feature switch.

Even then... Couldn't you just add a post-processing step like

const withoutEmptySpaces = string.replace(/^\s+$/gm,"")

or use prettier to format the result? Seems easier to me than changing Handlebars.

@bdiz
Copy link
Author

bdiz commented Mar 30, 2019

Thanks for taking a look.

Here is some output which is bit more illustrative of our use case.

image

We really see this as a bug in the indentation algorithm. We feel it's not clean code to add indent on the blank line of the function above. Anybody generating code that will be linted would benefit from this fix.

I'm not understanding this being a breaking change. What use case would want indent for blank lines of a partial?

We could post-process, as you recommend, but we use this extensively at https://www.uvmgen.com and since we are reactively generating code on the front end, performance is important to us. This does not add any performance overhead to handlebars since it already has a condition checking for blank lines as it adds indent.

Adding indent originally (#858) needed the preventDefault feature switch because indentation was a new feature. If you consider this a bug fix as we do, a feature switch may not be needed.

@nknapp
Copy link
Collaborator

nknapp commented Mar 31, 2019

I have become very cautious with changes and very cautious with the assumption that "nobody would use Handlebars this way". The lack of a formal language specification makes it difficult to say what is a bug and what isn't. The large number of users makes it probable that one of them relies on exactly that behavior that you want to change, even if it seems insignificant to you and me.

You are right that when using Handlebars to create JavaScript, Java or alike, it seems natural to skip indent on blank lines. In Python, the majority also seems to prefer blank lines without indent. But there are some that say blank lines should be indented (https://stackoverflow.com/a/25002433).

Without any target file type in mind, treating blank lines and non-blank lines the same seems natural to me, so I don't consider this a bug.

I cannot say for sure that your change does or doesn't break any build, except by publishing a new version with the change. Then, if just one user complains, I will have to revert the change and publish a new version. The user may have had an hour of debugging until she finds out that a change in Handlebars is responsible for her test failures. And I have spent at least an hour publishing two patch versions (publishing is not completely automated yet).

But since this affects that language, I'd like to get @wycats opinion, too.

@nknapp
Copy link
Collaborator

nknapp commented Mar 31, 2019

Oh, it just came to my mind that if this is not a bug, than it is a new feature that changes the language. I promised @wycats not to allow any new features, until there is a formal language specification.

Another reason to delegate this decision to @wycats

@wycats wycats self-assigned this Apr 11, 2019
@wycats
Copy link
Collaborator

wycats commented Apr 11, 2019

@nknapp thanks for being cautious!

I tend to think that this is on the border between a feature and a bug, but you're right that it's very likely to end up being a breaking change.

I wouldn't be opposed to collecting use-cases for a new revision of the whitespace stripping algorithm (indentation sensitivity comes to mind as a justification for a revision), and in that context, I think this change would make sense.

@bdiz bdiz force-pushed the 4.x branch 2 times, most recently from 8dbf5e2 to f541f3e Compare October 24, 2022 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants