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 cell() and element() twig functions #29

Merged
merged 2 commits into from
Apr 5, 2020
Merged

Conversation

othercorey
Copy link
Member

@othercorey othercorey commented Apr 3, 2020

#22

@othercorey othercorey added this to the 1.0 milestone Apr 3, 2020
@othercorey
Copy link
Member Author

I wonder if it is safe to cache the TwigEnvironment instance in a static variable so Cell's don't create new ones for every render.

@ADmad
Copy link
Member

ADmad commented Apr 4, 2020

I wonder if it is safe to cache the TwigEnvironment instance in a static variable so Cell's don't create new ones for every render.

I think it should be.

@othercorey othercorey force-pushed the element-function branch 2 times, most recently from fd2ad84 to 3074f80 Compare April 4, 2020 12:07
@othercorey
Copy link
Member Author

Twig Environment instance is now cached. If we want to test creating new Environment instances later, we will need to add a way to clear it.

ADmad
ADmad previously requested changes Apr 4, 2020
tests/test_app/templates/cell.twig Outdated Show resolved Hide resolved
@ADmad
Copy link
Member

ADmad commented Apr 4, 2020

The readme needs to be updated with new function usage examples.

@othercorey
Copy link
Member Author

I was going to re-write it once we've finished our cleanup since we keep adding/changing new functions. Is that ok?

@ADmad
Copy link
Member

ADmad commented Apr 4, 2020

I think @codeCoverageIgnore should be added to the node and parser classes for element and cell to avoid the coverage drop.

@othercorey
Copy link
Member Author

I think @codeCoverageIgnore should be added the node and parser classes for element and cell to avoid the coverage drop.

I was thinking about adding a single deprecation notice to each and adding a unit test that verify deprecation warning for the element tag.

I don't know if we want to spam warnings while people transition though.

@ADmad
Copy link
Member

ADmad commented Apr 4, 2020

I don't know if we want to spam warnings while people transition though.

Yeah that doesn't sound too nice. I am leaning towards just documenting the deprecations.

@othercorey
Copy link
Member Author

I added a unit test to cover the deprecated tags and all of the node code.

@ADmad ADmad linked an issue Apr 5, 2020 that may be closed by this pull request
@ADmad ADmad merged commit 08d7b8c into master Apr 5, 2020
@ADmad ADmad deleted the element-function branch April 5, 2020 19:33
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.

Use function notation for elements and cells
2 participants