Template Cleanup

[Korusuke]

As of now I have just done for python Portfile.

It works but there are issues with spacing, as for this code we get the following output
for upt:

    depends_build-append \
                    port:py${python.version}-setuptools

    depends_lib-append \
                port:py${python.version}-spdx-lookup


    livecheck.type  none

which is right but for upt-cpan it is:

    depends_build-append \
                    port:py${python.version}-setuptools

    depends_lib-append \
                port:py${python.version}-requests \
                port:py${python.version}-upt
    depends_test-append \
                port:py${python.version}-requests-mock

    livecheck.type  none

I already tried many different combination both logically and brute forcing but none gets the expected result.
The if statement around macro call is totally unnecessary and is just there as it was a possible solution as per this.

I will try a bit more if this can be fixed.

[reneeotten]

@Korusuke cool, this will definitely help with readability of the templates!

However, I would say the indentation for depends_lib and depends_test is actually wrong in both cases (at least it's different than for depends_build and livecheck). In the first example there shouldn't be an newline before the livecheck, whereas in the second there should be a newline before the test-dependencies as you noted.

Have you tried fiddling around with the lines below in the jinja2.Environment?

            trim_blocks=True,
            lstrip_blocks=True,
            keep_trailing_newline=True,

[Korusuke]

Have you tried fiddling around with the lines below in the jinja2.Environment?

            trim_blocks=True,
            lstrip_blocks=True,
            keep_trailing_newline=True,

Yes I did but to no luck, turning trim_block to False introduces a whole lot of extra blank lines.

[Korusuke]

Ok so changing a bunch of things "it works" technically but in my view the readability of the base portfile now is next to none.

I will try a bit more if its possible in some other way.

[reneeotten]

You will need to add a def jinja2_reqformat(self, req): for all MacPortsPackage classes not only for the Python one as you have done now. If we would merge this PR right now it would not work for frontends other than upt-pypi, for example with upt-cpan it throws an error:

AttributeError: 'MacPortsPerlPackage' object has no attribute 'jinja2_reqformat'

[Steap]

Yep. You could run a little script that runs "upt -b macports" with all known frontends and makes sure it does not crash in order to catch these issues :)

[reneeotten]

there are still issues with empty lines; I know that these templates are a bit cryptic, but the Portfiles we output should be correct

[Korusuke]

Ok I checked and there were 2 empty lines in ruby portfile after checksums and the 2 empty lines at the end in ruby and perl portfiles.

Now everything should be fixed.

[reneeotten]

Ok I checked and there were 2 empty lines in ruby portfile after checksums and the 2 empty lines at the end in ruby and perl portfiles.

Now everything should be fixed.

no, it's still not correct. There are empty lines in the ruby template within the if {${subport} ne ${name}} { block and the if {${perl5.major} != ""} { in the perl template.

[Korusuke]

Still there is one empty line remaining at end of if block in perl and ruby ports.

[reneeotten]

okay, I will take a look at this myself then - you should focus on more useful things ;)

I had fixed the ruby template this afternoon in a few minutes, but now you force-pushed all kind of other changes again so I can't tell anymore what has changed. If I have permission to push to this branch I will do so, otherwise I'll put a copy of the corrected templates somewhere so that you can take a look at them.

[reneeotten]

@Korusuke @Steap these templates are a serious pain in the ***.... It took quite a bit of time throughout the weekend to get it to a state where I think it's consistent/correct for all things I have tried (~250 random ports of the Python/Perl/Ruby categories). I ended up with starting from the current master and adding stuff from this PR in there and then fiddling around with it...

I have moved around some stuff in the templates, corrected the whitespacing/newlines, and a few other things that didn't work correctly. One of them being the build/config dependencies in the Perl template, which didn't work correctly if either config or build were not present (e.g., libwww-perl). Another one is making sure that jinja2_reqformat uses lowercase port names for Python (and npm) which wasn't done correctly in this PR. We eventually will want to have one function to get the name in ??-<name> formatted correctly in upt_macports.py so that this conversion doesn't happen in all kind of different places (i.e., for the name, directory, and dependencies), but let's do that at a later stage (and in another PR).

I have left the kind-of-messy commits in my branch so that you can follow it. I am not sure how you want to proceed? Why don't you start by taking a look at it and then you can either take my commits, squash some of them, and perhaps merge parts in here?

[reneeotten]

@Korusuke how far are you with this? It would be good to get this merged first, then you can update #39 to follow the same strategy.

[codecov]

Codecov Report

Merging #38 into master will decrease coverage by 1.93%.
The diff coverage is 42.85%.

@@            Coverage Diff            @@
##           master     #38      +/-   ##
=========================================
- Coverage   87.74%   85.8%   -1.94%     
=========================================
  Files           1       1              
  Lines         155     162       +7     
  Branches        7       7              
=========================================
+ Hits          136     139       +3     
- Misses         19      23       +4

Impacted Files	Coverage Δ
upt_macports/upt_macports.py	85.8% <42.85%> (-1.94%)	⬇️

---

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 51335cc...70aa656. Read the comment docs.

[reneeotten]

okay, I see you took the code from my branch. Did you do some additional testing yourself to make sure everything works as you want to? If you agree/confirm here that this indeed works as intended I'll merge it.

[Korusuke]

Yes I did test it and went over the whole code...I had considered doing this way(using if else for every scenario) but personally I don't really like it much but seeing nothing else works, I guess this is the only way we have :)

[reneeotten]

Yes I did test it and went over the whole code...I had considered doing this way(using if else for every scenario) but personally I don't really like it much but seeing nothing else works, I guess this is the only way we have :)

It would be nice to solve in another way, but I think we both agree that it seems quite impossible.... In the end of the day, I think these jinja2 templates are intended for HTML code and there some extra whitespace doesn't really matter. In any case, this solution works and give the output we want so I am fine with some if.... else statements, and it's still clear to follow so let's not worry about it too much.

It would be nice to add some tests for the actual templates to make sure the fields/whitespaces are correct. One option would be to add "correct" templates for a a few ports in every category and compare the upt output to them (of course only the fields/whitespace and ignoring a possible change in version/checksums). Alternatively, we could probably just check the in the upt output directly that all expected fields and newlines are present. I would probably prefer the latter situation, but I don't know how hard that would be to do. In that case, we if someone decides to fiddle around with the templates we don't run the risk that things break unnoticed.

[Korusuke]

Ok I'll see how we can add tests but I guess that can be done in an another PR.
We should merge this now.

[reneeotten]

@Steap any concerns with merging this?

Kind of the only thing I am not a big fan of is the handling of the build/config dependencies in the Perl template. But the readability isn't too bad and I cannot figure out an easier, robust way of doing this - at least not in the template. I guess one could merge the config/build dependencies in the backend code, but I am not sure if this is more useful...

[reneeotten]

okay, I am going to merge this now.