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

Add types tag #4235

Closed
wants to merge 11 commits into from
Closed

Add types tag #4235

wants to merge 11 commits into from

Conversation

drjayvee
Copy link
Contributor

The is a draft implementation for #4165.

@drjayvee
Copy link
Contributor Author

I just force-pushed a fix to the docs

@drjayvee
Copy link
Contributor Author

This PR isn't complete yet. I forgot to add a TypesTokenParser (and tests).

doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
@drjayvee
Copy link
Contributor Author

drjayvee commented Aug 28, 2024

To quote my last commit message:

Move parsing of {name: 'string'} mappings to ExpressionParser

This also adds support for optional keys as discussed in #4165.

I really don't like using a static method, but I couldn't figure out how
else to call the method in tests. See the notes on the method itself.

Any help is appreciated!

@drjayvee
Copy link
Contributor Author

drjayvee commented Aug 29, 2024

I don't understand the CI error:

Remaining self deprecation notices (2)

  2x: Since twig/twig 3.12: The "tag" constructor argument of the "Twig\Node\TypesNode" class is deprecated and ignored (check which TokenParser class set it to "null"), the tag is now automatically set by the Parser when needed.
    1x in TypesTest::getTests from Twig\Tests\Node
    1x in TypesTest::testConstructor from Twig\Tests\Node

That test does not pass the tag argument.

Update: I also can't reproduce this on my local dev stack.

I think the problem is that the TypesNode constructor passes tag to parent::__construct(). I'll remove that and push again.

src/Node/TypesNode.php Outdated Show resolved Hide resolved
src/ExpressionParser.php Outdated Show resolved Hide resolved
src/ExpressionParser.php Outdated Show resolved Hide resolved
src/ExpressionParser.php Outdated Show resolved Hide resolved
tests/ExpressionParserTest.php Outdated Show resolved Hide resolved
src/Node/TypesNode.php Outdated Show resolved Hide resolved
src/ExpressionParser.php Outdated Show resolved Hide resolved
src/ExpressionParser.php Outdated Show resolved Hide resolved
src/TokenParser/TypesTokenParser.php Outdated Show resolved Hide resolved
Copy link
Contributor

@ruudk ruudk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👏

@drjayvee drjayvee requested a review from fabpot August 29, 2024 14:04
Copy link
Contributor

@fabpot fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've added some minor comments.

CHANGELOG Outdated Show resolved Hide resolved
doc/tags/types.rst Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
doc/tags/types.rst Outdated Show resolved Hide resolved
src/TokenParser/TypesTokenParser.php Outdated Show resolved Hide resolved
src/TokenParser/TypesTokenParser.php Outdated Show resolved Hide resolved
@fabpot
Copy link
Contributor

fabpot commented Aug 29, 2024

Thank you @drjayvee.

fabpot added a commit that referenced this pull request Aug 29, 2024
This PR was squashed before being merged into the 3.x branch.

Discussion
----------

Add types tag

The is a draft implementation for #4165.

Commits
-------

67dabd8 Add types tag
@fabpot
Copy link
Contributor

fabpot commented Aug 29, 2024

Not sure why Github didn't work as expected here. I'm closing but it's merged.

@VincentLanglet
Copy link
Contributor

Hi @drjayvee,

Is there a reason to not support

{% types {
        'foo': 'bool',
        'bar'?: 'int',
    } %}

too ? Like it's done for classic objects {% set bar = {'foo': 42} %}.

@ruudk
Copy link
Contributor

ruudk commented Nov 5, 2024

I guess we did not consider this as it looks a bit ugly for optionally. But sure, we could still accept this. Especially as it's better in line with mappings.

I do hope we can turn of the quoting for the types tag in Twig-CS-Fixer though 😉

@stof
Copy link
Member

stof commented Nov 5, 2024

@VincentLanglet variable names need to be valid identifiers anyway to be usable, so I don't see the benefit of supporting string literals as keys there instead of only identifiers.
This syntax is not an actual hash anyway (the parser does not allow arbitrary expressions)

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

Successfully merging this pull request may close these issues.

5 participants