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

Whitespace improvements #2925

Merged
merged 3 commits into from Apr 3, 2019
Merged

Whitespace improvements #2925

merged 3 commits into from Apr 3, 2019

Conversation

@fabpot
Copy link
Contributor

@fabpot fabpot commented Apr 3, 2019

This pull request introduces a new whitespace control modifier in addition to the - one. The new ~ modifier consume whitespace excluding newlines.

closes #2924, closes #1005, closes #1423, closes #1569, and many already closed ones.

@fabpot fabpot changed the base branch from 2.x to 1.x Apr 3, 2019
@fabpot fabpot force-pushed the fabpot:whitespace-improvements branch from 984dbb2 to 3d0ed0f Apr 3, 2019
@fabpot fabpot mentioned this pull request Apr 3, 2019
@fabpot
Copy link
Contributor Author

@fabpot fabpot commented Apr 3, 2019

Note that the new modifier works on both sides (unlike the previous attempt of others PRs on the same matter).

@fabpot
Copy link
Contributor Author

@fabpot fabpot commented Apr 3, 2019

If you want to review the PR, I suggest you to read each commit, which is much easier.

src/Lexer.php Outdated Show resolved Hide resolved
doc/templates.rst Show resolved Hide resolved
doc/templates.rst Outdated Show resolved Hide resolved
} else {
// whitespace_line_trim detected ({%~, {{~ or {#~)
// don't trim \r and \n
$text = rtrim($text, " \t\0\x0B");

This comment has been minimized.

@stof

stof Apr 3, 2019
Member

shouldn't this use the whitespace_line_chars option ?

} else {
// whitespace_line_trim detected ({%~, {{~ or {#~)
// don't trim \r and \n
$text = rtrim($text, " \t\0\x0B");

This comment has been minimized.

@stof

stof Apr 3, 2019
Member

shouldn't this use the whitespace_line_chars option ?

This comment has been minimized.

@fabpot

fabpot Apr 3, 2019
Author Contributor

no, whitespace_line_chars are quoted

@stof
Copy link
Member

@stof stof commented Apr 3, 2019

To optimize the lexing, I think we should be able to make lots of quantifiers possessive in these regexes (\s* becoming \s*+). I think it would work fine for the default syntax.
But this change might be risky for cases where the syntax is customized (they might be inventing crazy delimiters). I'm not even sure we should keep these options in 3.x (TwigBundle never exposed them in Symfony projects as that would break the ecosystem. Changing the syntax creates a new templating language).

Copy link
Contributor

@javiereguiluz javiereguiluz left a comment

👍

I'm not a big fan of using ~ because it's too similar to - (it will be difficult to differentiate them depending on the font family and font size used in the editor) but I can't think of anything better.

Jinja uses - and +, but their use case is different: http://jinja.pocoo.org/docs/2.10/templates/#whitespace-control

@fabpot
Copy link
Contributor Author

@fabpot fabpot commented Apr 3, 2019

I thought about using quantifiers possessive, but did not do it for BC.

@fabpot fabpot force-pushed the fabpot:whitespace-improvements branch from 3d0ed0f to 7327f5d Apr 3, 2019
@fabpot
Copy link
Contributor Author

@fabpot fabpot commented Apr 3, 2019

@javiereguiluz I would argue that using similar characters is a good thing as they are doing almost the same thing :)

@fabpot fabpot force-pushed the fabpot:whitespace-improvements branch from 7327f5d to 96eab47 Apr 3, 2019
Copy link

@brunoais brunoais left a comment

Solution for the operator to do something like in my proposal:
2.x...brunoais:linespaceControl

src/Lexer.php Show resolved Hide resolved
src/Lexer.php Show resolved Hide resolved
src/Lexer.php Show resolved Hide resolved
@fabpot fabpot merged commit 96eab47 into twigphp:1.x Apr 3, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabpot added a commit that referenced this pull request Apr 3, 2019
This PR was squashed before being merged into the 1.x branch (closes #2925).

Discussion
----------

Whitespace improvements

This pull request introduces a new whitespace control modifier in addition to the `-` one. The new `~` modifier consume whitespace excluding newlines.

closes #2924, closes #1005, closes #1423, closes #1569, and many already closed ones.

Commits
-------

96eab47 added support for a new whitespace trimming option
7e3ec0f made Lexer regexes more readable
591da36 simplified Lexer regexes
@fabpot fabpot mentioned this pull request Apr 5, 2019
@fabpot fabpot deleted the fabpot:whitespace-improvements branch Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.