jrjsmrtn commented 4 years ago

Use summary for description, description for long_description Backslash double quotes in description fields

codecov[bot] commented 4 years ago

Codecov Report

Merging #57 into master will decrease coverage by 0.01%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   90.64%   90.62%   -0.02%     
==========================================
  Files           1        1              
  Lines         171      192      +21     
  Branches       10       15       +5     
==========================================
+ Hits          155      174      +19     
  Misses         16       16              
- Partials        0        2       +2

Impacted Files	Coverage Δ
upt_macports/upt_macports.py	`90.62% <0%> (-0.02%)`	:arrow_down:

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 216634f...808bf1b. Read the comment docs.

jrjsmrtn commented 4 years ago

UPT frontends transmit package descriptions in two fields:

a short, one-line summary,
a long, multiline description.

Unfortunately, the current upt-macports implementation maps only the summary frontend field to both the (short) description and long_description Portfile fields.

Moreover, the current upt-rubygems frontend implementation extracts only the long, multiline description RubyGem field and transmit it as summary to the backends. This means that packaging a RubyGem results in long, multiline description and long_description Portfile fields.

This pull request fix the summary --> description and the description --> long_description mapping.

It also backslash double quotes in those description fields, prevention Portfile parsing errors.

The corresponding https://framagit.org/upt/upt-rubygems/merge_requests/2 pull request fixes the RubyGemsFrontend implementation to extract both (short) summary and (long) description and correctly map them to the backend fields.

reneeotten commented 4 years ago

thanks @jrjsmrtn for the PR; these changes make sense to me.

Let me take a look at what the different frontends support and test some packaging with the different frontends to see if this all works as intended. For example I am not sure if there will always be a description present; so perhaps we need some additional checks in the template for empty values. Also, let's see what @Steap thinks about the changes to upt-rubygems.

I'll follow-up soonish!

reneeotten commented 4 years ago

@jrjsmrtn @Steap I looked at the result for a random selection of 250 Python and Perl ports and don't think we can/should generalize it in the way you suggest.

In the case of Perl ports, the long_description is now always empty, consistent with the fact that the upt-cpan frontend only sets the summary field to the abstract from the metacpan json data. Looking at the json data, it seems to me that there is no obvious candidate for long_description present, or am I missing something?

For many Python ports the long_description is set to the content of the README file, which is almost always waaay too much text to include in the Portfile. So I wouldn't want to set the long_description to that and have the maintainer remove all of the additional text. So here I would prefer as well to keep it by default as ${description} and leave it to the maintainer to change if needed.

Perhaps the rubygems actually does give a decent long_description for the majority of the packages. If that's the case, we can likely set the long_description field only when using the upt-ruby frontend and leave it as-is for the others. What do you think?

Steap commented 4 years ago

@reneeotten Depending on how useful metadata is in every single project, you could edit all the lanuguage-specific templates and do something different for each one of them.

reneeotten commented 4 years ago

@reneeotten Depending on how useful metadata is in every single project, you could edit all the lanuguage-specific templates and do something different for each one of them.

Yep, that's indeed pretty much what I was suggesting. I would argue it's not really useful for Perl and Python modules; I haven't checked yet for Ruby. If it does give useful information for Ruby we can likely just do something like this (untested):

{% if pkg.upt_pkg.frontend == 'rubygems' -%}
long_description    pkg.upt_pkg.description |trim |replace('\n',' \\\n') |replace('\"','\\\"') |indent(20, false) %}
{%- else -%}
long_description    ${description}
{% endif %}

jrjsmrtn commented 4 years ago

@reneeotten Depending on how useful metadata is in every single project, you could edit all the lanuguage-specific templates and do something different for each one of them.

I updated my pull request to do just that.

I added a default descriptions block in base.Portfile. The descriptions block in {perl,python,ruby}.Portfile can then be customized by replacing the {{ super() }} call.

jrjsmrtn commented 4 years ago

I pushed another change to simplify the customization of descriptions blocks.

In the case of Perl ports, the long_description is now always empty, consistent with the fact that the upt-cpan frontend only sets the summary field to the abstract from the metacpan json data.

The long_description definition in base.Portfile falls back to ${description} if it's empty so we are covered.

Looking at the json data, it seems to me that there is no obvious candidate for long_description present, or am I missing something?

No, I don't think so.

The only candidate I see is that some modules have a README.md with a Description section. But: 1. not all modules have such a README in a parseable format, 2. that section can be very long and 3. getting to that README complicates things a lot. I'm not sure it's worth the trouble for now.

For many Python ports the long_description is set to the content of the README file, which is almost always waaay too much text to include in the Portfile. So I wouldn't want to set the long_description to that and have the maintainer remove all of the additional text. So here I would prefer as well to keep it by default as ${description} and leave it to the maintainer to change if needed.

The descriptions block in python.Portfile is now customized to always set long_description to ${description}.

Beware that there is a small gotcha in upt-rubygems that results in upt_package.description being set to a "None" string in upt-macports Jinja2 templates. I'll also send pull request for those.

reneeotten commented 4 years ago

I updated my pull request to do just that.

I added a default descriptions block in base.Portfile. The descriptions block in {perl,python,ruby}.Portfile can then be customized by replacing the {{ super() }} call.

To me it seems an unnecessary complication to add code for this to all templates. I don't think we need to customize anything by replacing the {{ super() }} call for individual templates, or? What's wrong with just adding one, simple if statement to the base profile only?

jrjsmrtn commented 4 years ago

What's wrong with just adding one, simple if statement to the base profile only?

I just followed the inheritance principle:

the generalized behavior, meaning a short, one line description in the summary-->description fields, a long, multiline description in the description-->long_description fields, and summary-->long_description when there is no long description, goes into the default, generic base.Portfile;
specialized behaviors, for example always using the short summary for both description/long_description fields in PyPI ports, go in the specialized templates.

You are right, my patch is (a bit) longer. But putting specializations in both the base and the specialized templates just seems unnatural to me... :-q

Just tell me :-)

mojca commented 4 years ago

Thank you for working on this, including putting gems to real stress test by packaging useful software. No time to test right now, but thanks in advance to everyone involved. I hope @reneeotten or @Korusuke have time to also update upt as well as merge your contributions. Thanks.

reneeotten commented 4 years ago

@jrjsmrtn sorry for the delay... Okay, so indeed rubygems does give a useful long_description in many cases, so we should indeed add this to the template.

I have a few suggested changes:

make use of striptags to remove newline and tabs and use wordwrap for long_description
leave the default in the base.template to set long_description ${description} since we use that for 2 out of the three templates, and override that block in ruby.Portfile.

If you're okay with that I can add the two commits that do this to this PR and you can take a look. If needed we can make some more adjustments, and once we all agree just "squash and merge" this. Let me know what you think!

jrjsmrtn commented 4 years ago

I have a few suggested changes: make use of striptags to remove newline and tabs and use wordwrap for long_description

I tried something like that but some package descriptions have lists of items (rake), some have citations (minitest), some have Markdown markups,… and the results were worse. So, for now, I decided to keep the current conversion as it is.

G.

reneeotten commented 4 years ago

I have a few suggested changes: make use of striptags to remove newline and tabs and use wordwrap for long_description I tried something like that but some package descriptions have lists of items (rake), some have citations (minitest), some have Markdown markups,… and the results were worse. So, for now, I decided to keep the current conversion as it is. G.

We are not trying to solve every possible corner-case, but find a solution that works for 99% of the time. It's up to the maintainer to take care of the fine-tuning of the Portfile.

I would consider for both of the packages you mention the long_description provided by upstream unsuitable for MacPorts anway: we don't want to have paragraphs of text (nor markdown and such). Additionally, we likely don't want to keep the wrapping used by upstream as they commonly wrap to be <80 characters; since we start our long_description at position 21, we will certainly go over that limit. Finally, with your proposed changes if description and long_description are the same, you would still add the text to both of them (that's not what we want, in that case it should be long_description $description), and it doesn't keep the newlines where we want them.

I think the descriptions-block as you proposed are indeed a good idea. As said above, I would just want to keep the default in the base.Portfile as we have it now and only override it for rubygems.

Of course you can do locally whatever suits your needs, but I am trying to work with you here to get a solution merged. So, the answer I kind-of anticipated was "yes, please add your commits here and let's take a look".

jrjsmrtn commented 4 years ago

@mojca:

Thank: you for working on this, including putting gems to real stress test by packaging useful software. No time to test right now, but thanks in advance to everyone involved. I hope @reneeotten or @Korusuke have time to also update upt as well as merge your contributions. Thanks.

You're welcome. Thanks for your work :-)

jrjsmrtn commented 4 years ago

@reneeotten:

I think the descriptions-block as you proposed are indeed a good idea. As said above, I would just want to keep the default in the base.Portfile as we have it now and only override it for rubygems.

Sorry, busier week than expected. It's done.

reneeotten commented 4 years ago

well, you have done part of what I suggested. I still think we should change the wrapping and replacement of tags.

Also, as @Steap suggested in another PR, it might be better here as well to do some of this in the Python backend code so that we don’t need to over complicate the templates.

Anyway, I am flying out for holidays now so I don’t expect to have much time for this in the next weeks.

macports / upt-macports

Fix description fields #57

Codecov Report