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

Adds templates config file for managing templates #21

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

suhaboncukcu
Copy link

With this improvement;

  • You can use the plugin as it was without changing anything.
  • You can create templates in a config file so you don't need to create an instance of utility class if you don't need it.
  • Check new ways for templating on readme.

@codecov-io
Copy link

codecov-io commented Oct 18, 2016

Current coverage is 80.29% (diff: 100%)

No coverage report found for master at 585c19f.

Powered by Codecov. Last update 585c19f...693fa7a

@bobmulder bobmulder mentioned this pull request Oct 19, 2016
@bobmulder
Copy link
Contributor

Okay @suhaboncukcu, first of all thank you very much for your contribution! It feels good that people like it and are motivated to improve it.

However, I do have some feedback:

  • You used 5 commits to reach this PR; while you could have done it in less I guess (according to the white lines ;)). Try to minimize it.
  • I like the configure stuff, but can't we add some tests to be sure default or added templates aren't overruled?
  • Templates can already be added through the Configure class, right? Shouldn't we document that? *just a question

@bobmulder bobmulder self-assigned this Oct 19, 2016
@suhaboncukcu
Copy link
Author

Thanks for the comments @bobmulder!

1 - I'm used to commit every change I create and didn't think that the files I didn't touch had some errors :) How should I proceed? Should I close this one and create another PR with another fork? (I should have worked with another branch indeed though, my bad!)

2 - I think, if one uses Configure class, it can always be overruled. Could you give an example for this?

3 - Could be. If there is just one template, I guess one could want to define the template on bootstrap in Configure class and go on. I'm not sure.

@bobmulder
Copy link
Contributor

1 - It's okay for now, lets keep this PR :)

2 - Example could be; what if I added some templates via my Configure class (using config.php), and later on load an external config file. What will happen, how will they be merged? By adding tests, we just will be sure this works :)

3 - To be complete we would be good to add it to the docs I think. Agree?

👍

@suhaboncukcu
Copy link
Author

I'm sorry for my dumbness on this; tests seem to be my weak spot. How can I do that? Would you mind getting in touch on slack/gitter/skype/ etc?

I can update the docs but then again, shouldn't this be in the "black box" for the plugin? I think developers using the plugin shouldn't register the templates themselves via Configure class. There could be an option to -just for an instance- encrypt the templates via a function within the plugin before adding them to Configure. Or let's say we've added an option to be configured to get templates as json objects instead of texts. I think developers should be discouraged to use Configure class to register templates. It's just my opinion though I can add the change in this PR if you are sure about adding Configure description to docs. :)

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.

3 participants