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 Twig and Plates extensions #7

Merged
merged 1 commit into from
Sep 12, 2018
Merged

Conversation

dborsatto
Copy link
Contributor

@dborsatto dborsatto commented Sep 12, 2018

This PR adds support for integrating this library with templating engines Twig and Plates.

@codecov
Copy link

codecov bot commented Sep 12, 2018

Codecov Report

Merging #7 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master     #7   +/-   ##
=======================================
  Coverage       100%   100%           
- Complexity      175    181    +6     
=======================================
  Files            60     62    +2     
  Lines           522    540   +18     
=======================================
+ Hits            522    540   +18
Impacted Files Coverage Δ Complexity Δ
src/Bridge/TwigExtension.php 100% <100%> (ø) 3 <3> (?)
src/Bridge/PlatesExtension.php 100% <100%> (ø) 3 <3> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b26b93...46a7f10. Read the comment docs.

@dborsatto dborsatto force-pushed the feat/bridge-integrations branch 2 times, most recently from 75bed4e to f40c44c Compare September 12, 2018 09:53
@dlitvakb
Copy link

I'm not entirely sure that we should provide this integrations within the package, as it means adding external dependencies that may pull in A LOT of framework dependencies that are strictly not necessary for most people that are going to be using this library.

I would go for a separate package for the helpers, or to include it in the current extensions we already have.

@dborsatto
Copy link
Contributor Author

dborsatto commented Sep 12, 2018

Actually, they don't add any dependency to users of this library. The dependencies are only included when working on this library, but if you include it in your project, they won't be added (they are in the require-dev section of the config file, not in the require).

It's literally zero cost for the user 🙂

@dlitvakb
Copy link

I'm still a bit sceptic about this, as users may have dependency issues if they choose the incorrect bridge, but if you think it's going to be alright, then I'm ok with your decision.

@dborsatto
Copy link
Contributor Author

Sorry, but there's literally no dependency to this.

The fact that I implement these classes here does not cause any dependency inclusion for the user. Actually, if the user tries to work with one of these bridge classes without having manually installed the required templating engine, they will receive an error because they actually lack the dependency.

@dborsatto dborsatto merged commit 88b8202 into master Sep 12, 2018
@dborsatto dborsatto deleted the feat/bridge-integrations branch September 12, 2018 12:41
@dlitvakb
Copy link

That's exactly what I meant though, having the dependency error is not good. Because the unaware user, instead of changing the bridge to the correct one, they may install dependencies to shut the thing up.

@dborsatto
Copy link
Contributor Author

No, because it will never cause an error if you don't try to use the bridge. This library works completely fine without it, it's built not to force any dependency.

The only error would happen if you intentionally use the TwigExtension and you don't have Twig installed, but if you do so, it's not this library's fault to be honest. It can't cause any sort of conflict because it doesn't define a dependency, and it's also how all packages implement optional dependencies: it's just there, and it's so explicitly opt-in that you must know what you're doing if you use it...

@dborsatto
Copy link
Contributor Author

This basically letting the user know "Hey, if you already use Twig or Plates in your project, there's this handy extension you can just use", nothing else.

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