Skip to content
This repository has been archived by the owner on Apr 5, 2020. It is now read-only.

Added docblocks #1

Closed
wants to merge 2 commits into from
Closed

Added docblocks #1

wants to merge 2 commits into from

Conversation

odan
Copy link
Contributor

@odan odan commented Oct 30, 2018

No description provided.

@codecov
Copy link

codecov bot commented Oct 30, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##             master     #1   +/-   ##
=======================================
  Coverage       100%   100%           
  Complexity       34     34           
=======================================
  Files             1      1           
  Lines            70     70           
=======================================
  Hits             70     70
Impacted Files Coverage Δ Complexity Δ
src/PHPTemplate.php 100% <100%> (ø) 34 <34> (ø) ⬇️

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 625de34...38412c5. Read the comment docs.

Copy link
Owner

@Ayesh Ayesh left a comment

Choose a reason for hiding this comment

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

Hi Daniel,
Thanks a lot for the PR. It certainly makes many things very clear now from the docblock.

Because we use PHP 7.2, I tried to use as little as possible doc blocks, and I personally would prefer not to use @param and @return where they are enforced with parameter and return type hints.

The escape methods for HTML, URL, and helper for the attributes really could use your comments, and I'm sure they are immensely helpful. Would you mind if we not use doc block @param and @return for methods where it is enforced in the type hint and the parameter name is clear enough what the parameter values should be?

Thanks again for the PR. I started this project just to separate something I wanted for a small project, and in a field with several template engines, some even with their own language and many features sprinkled everywhere, I am glad this deserved got your attention and valuable time.

src/PHPTemplate.php Show resolved Hide resolved
src/PHPTemplate.php Show resolved Hide resolved
* Get view data by key name.
*
* @param string $key The key
* @return mixed|null The value
Copy link
Owner

Choose a reason for hiding this comment

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

👍👍👍👍👍❤

src/PHPTemplate.php Show resolved Hide resolved
src/PHPTemplate.php Show resolved Hide resolved
src/PHPTemplate.php Show resolved Hide resolved
@odan
Copy link
Contributor Author

odan commented Oct 31, 2018

I try to write code as consistent as possible and DocBlocks are part of it for me. Although data types can be strictly defined since PHP 7.2, parameters and return values are often in need of explanation. It's nice that we finally have strict types, but we humans have to be able to understand the code too. :-)

@odan odan closed this Nov 21, 2018
@Ayesh
Copy link
Owner

Ayesh commented Nov 22, 2018

Hi @odan - I'm so sorry it took me a long time to go with this PR.

I believe the .gitignore file should only relate to the project files and IDE-specific ignore rules should go to the developer's ~/.gitignore file. I cherry-picked the main commit and added a bit more information into it and just released a new v1.0.2 version with all changed bundled.

Again I'm sorry for the koala-like maintainership and thanks again for this PR!

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

Successfully merging this pull request may close these issues.

None yet

2 participants