TemplateEngine improvements, updated template.md

[nozmore]

I updated the template to showcase #150 and #154. Also expanded template_engine to support the call operator with methods that return a list and support to call methods within a report utility class. Updated template.md with examples of each.

Updated tests and ensured tests passed. Also updated report test to write output_current.md At some point tests could use this but for now can be used to quickly commit the new expected value when the report changes. Added output_current.mdto.gitignore`

I also added an example in the template.md to get the parents of each Boundary, to see this I updated tm.py with a hierarchy of Boundaries. If we think this is too much for the intro tm.py I can back those changes out.

[nozmore]

oh also added entries in the Data table for carriedBy and processedBy.

[ghost]

Congratulations 🎉. DeepCode analyzed your code in 2.765 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

[nineinchnick]

I'm not convinced we need such a complicated template as an example. I believe it would confuse new users. We should send a clear message that the templating engine in pytm is very simple by design, and if it's not enough, users should override it and use a different one.

[nineinchnick]

@nozmore @izar WDYT about keeping two template examples? One simple, one more advanced?

[nozmore]

lol I put that in my reply to an earlier comment before reading this. Yes, I think we are viewing this report differently I would prefer to have a fairly complete report and then in docs have a simple version to go along with the simple.tm

[nozmore]

Why did my tests fail here? Is this executing against the test files in master or in the branch, locally everything passes and I didn't touch DFD logic.

[nozmore]

Sorry I disappeared, life got in the way. I fixed some issues and cleaned up the reports, merged in master. I think this is ready to be merged, well at least reviewed : )

[raphaelahrens]

Hi,

first I like the change and I wanted to add my thoughts.
Might it be a better idea to have extra repos for reports?
Since a fancy report does not have to be part of the software.
In addition one could also add build instruction and files in the repos which would be solely for the report.
For example I wanted to checkout if I could make a pdf report and if I find the time I thought about pushing it to a separate repo, which could then be linked to.
Also the users could fork repot to adjust it to their needs (people love company logos and names).

This does not really affect this PR but maybe future PRs which want to change the report template.

[izar]

I like the idea of more complex reports but I think they should go into a directory, not a separate repo, lest the two repos get out of sync version-wise.

[nineinchnick]

I'm still not convinced it's the right thing to do to extend this basic template engine. It's already non-trivial to write advanced templates in it. I vote for replacing it with jinja2.

[nozmore]

I'm still not convinced it's the right thing to do to extend this basic template engine. It's already non-trivial to write advanced templates in it. I vote for replacing it with jinja2.

If it wasn't already done I'd agree. Its done, functional, adds minimal changes to the engine, and moves pytm forward. Replacing with another library can still be done later or use something to render the JSON output.

This is my only open source project, how to tie breakers work?

[nineinchnick]

I'm still not convinced it's the right thing to do to extend this basic template engine. It's already non-trivial to write advanced templates in it. I vote for replacing it with jinja2.

If it wasn't already done I'd agree. Its done, functional, adds minimal changes to the engine, and moves pytm forward. Replacing with another library can still be done later or use something to render the JSON output.

This is my only open source project, how to tie breakers work?

I don't agree that changes proposed here are minimal.
@izar has they final word here. I can disagree and commit.

[izar]

Sorry for the late reply, I am a bit overwhelmed these days. I am reluctant to move into something like jinja2, even though I like it - I fear we "get married" to it moving forward, but it is a bit of an unfounded worry. I can't put my finger on it, but something bothers me. Here's what I propose - as @nozmore says, it is done. How about we move forward with this patch, and on the next "the template engine doesn't do X" we reassess - if jinja2 or some other engine do it and we find out the move is "easy" we go that way, otherwise we continue with this implementation. Acceptable compromise? Btw, I don't have the final word. I value you guys' opinion very much and I will never overrule a good argument, especially considering how much I always learn from each one of you. We are a democratic team.

…

On Wed, Oct 6, 2021 at 2:13 AM Jan Waś ***@***.***> wrote: I'm still not convinced it's the right thing to do to extend this basic template engine. It's already non-trivial to write advanced templates in it. I vote for replacing it with jinja2. If it wasn't already done I'd agree. Its done, functional, adds minimal changes to the engine, and moves pytm forward. Replacing with another library can still be done later or use something to render the JSON output. This is my only open source project, how to tie breakers work? I don't agree that changes proposed here are minimal. @izar <https://github.com/izar> has they final word here. I can disagree and commit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#155 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC2BALUMVVHP4UKBYQ3BF3UFPSHFANCNFSM42S74TRA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[nozmore]

Sounds good to me. I don't have plans to add any additional logic to the templating system. That said I do see additions to either the various internal classes or the report_util class when data needs to be formatted in a way for reporting.

For example the PR about custom properties using a dict, I have a method drafted locally that I will push to report_util once the template changes are merged that will enable looping over the dict and prevent KeyError exceptions when the key does not exist in the dict.

[ghost]

CodeSee Review Map:

Review in an interactive map

View more CodeSee Maps

Legend