Skip to content

Portindex2json: Make the license field consistent with "port info" command#6

Merged
arjunsalyan merged 1 commit into
macports:masterfrom
arjunsalyan:portindex2json-license
Jul 19, 2020
Merged

Portindex2json: Make the license field consistent with "port info" command#6
arjunsalyan merged 1 commit into
macports:masterfrom
arjunsalyan:portindex2json-license

Conversation

@arjunsalyan
Copy link
Copy Markdown
Member

@arjunsalyan arjunsalyan commented Jul 16, 2020

@arjunsalyan arjunsalyan force-pushed the portindex2json-license branch from 5698b25 to 9e0f449 Compare July 16, 2020 19:18
@arjunsalyan arjunsalyan requested a review from jmroot July 16, 2020 19:19
@ryandesign
Copy link
Copy Markdown
Contributor

I don't think I agree. Just as the data is in machine-readable format in the PortIndex and is formatted to be human readable in port info by MacPorts base, so too it is appropriate for the data to remain in machine-readable format in PortIndex.json and be formatted by the web app.

@arjunsalyan
Copy link
Copy Markdown
Member Author

A TCL list in portindex.json is only as good as a string. We can either make it a list of strings in JSON also or leave it as it is and do everything while rendering in the webapp.

Changing things in portindex.json will anyway require some work on the database also. So, we can close this and move ahead with macports/macports-webapp#235 which formats the license field when rendering the pages (unless there are any other suggestions).

@ryandesign
Copy link
Copy Markdown
Contributor

A TCL list in portindex.json is only as good as a string.

PortIndex is a Tcl file, so of course it contains Tcl data structures. PortIndex.json is supposed to be a JSON file, so I assumed it would have JSON representations of the data. I see now that for the license field it just contains the Tcl string. That seems wrong to me.

Taking the example of the py-setuptools port, its license in Tcl representation is MIT {PSF ZPL} {Apache-2 BSD}. In other words, it is a list of three items. The first item is the string "MIT". The second item is a 2-item list containing the strings "PSF" and "ZPL". The third item is a 2-item list containing the strings "Apache-2" and "BSD". So I'd expect PortIndex.json's license fields to contain whatever the JSON representation of such a nested list (array) structure is, and then I'd expect the web app to parse that and display it in human-readable form.

@jmroot
Copy link
Copy Markdown
Member

jmroot commented Jul 18, 2020

Just as the data is in machine-readable format in the PortIndex and is formatted to be human readable in port info by MacPorts base, so too it is appropriate for the data to remain in machine-readable format in PortIndex.json and be formatted by the web app.

I tend to agree, that would be a better approach. Among other things, it avoids limiting what else can be done with the JSON.

@arjunsalyan arjunsalyan force-pushed the portindex2json-license branch from 9e0f449 to 2585809 Compare July 18, 2020 06:17
@arjunsalyan arjunsalyan marked this pull request as draft July 18, 2020 06:20
@arjunsalyan
Copy link
Copy Markdown
Member Author

Taking the example of the py-setuptools port, its license in Tcl representation is MIT {PSF ZPL} {Apache-2 BSD}. In other words, it is a list of three items. The first item is the string "MIT". The second item is a 2-item list containing the strings "PSF" and "ZPL". The third item is a 2-item list containing the strings "Apache-2" and "BSD". So I'd expect PortIndex.json's license fields to contain whatever the JSON representation of such a nested list (array) structure is, and then I'd expect the web app to parse that and display it in human-readable form.

Thank you, I have done the changes and new license field now looks like this in JSON:

"license"          : ["MIT",["PSF","ZPL"],["Apache-2","BSD"]]

This should be the correct representation in JSON.

I am converting this PR to draft as it should not be merged before the needed changes get deployed on the webapp. I have used recursion to minimize the lines of code, not sure if is a good idea- that code block could use some feedback.

Copy link
Copy Markdown
Contributor

@ryandesign ryandesign left a comment

Choose a reason for hiding this comment

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

Thanks! It looks good to me. I don't know whether the recursion could be a problem. Not sure how Tcl handles recursion. But assuming it handles it at all, which it seems to, the license field won't be that complex that it would cause very much recursion.

Comment thread portindex2json/portindex2json.tcl Outdated
array unset json_portinfo
foreach key [array names portinfo] {
if {$key eq "categories" || $key eq "variants" || [string match "depends_*" $key]} {
if {$key eq "categories" || $key eq "variants" || $key eq "license" || [string match "depends_*" $key]} {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By the way you can use the in operator here, as in:

if {$key in [list categories variants license] || [string match "depends_*" $key]} {

@arjunsalyan arjunsalyan force-pushed the portindex2json-license branch from 2585809 to 1dda9ba Compare July 18, 2020 10:59
@arjunsalyan arjunsalyan force-pushed the portindex2json-license branch from 1dda9ba to 346d7c8 Compare July 18, 2020 11:00
@arjunsalyan arjunsalyan marked this pull request as ready for review July 18, 2020 11:30
@arjunsalyan
Copy link
Copy Markdown
Member Author

This can now be merged unless there is any feedback.

Copy link
Copy Markdown
Member

@jmroot jmroot left a comment

Choose a reason for hiding this comment

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

LGTM

@mojca
Copy link
Copy Markdown
Member

mojca commented Jul 19, 2020

So just to make it clear, here's how we would want the Tcl transformed (Tcl => json => human-readable form)?

  • MIT => ["MIT"] => MIT
  • MIT BSD => ["MIT", "BSD"] => MIT and BSD
  • {MIT BSD} => [["MIT", "BSD"]] => MIT or BSD
  • MIT {PSF ZPL} {Apache-2 BSD} => ["MIT", ["PSF", "ZPL"], ["Apache-2", "BSD"]] => MIT and (PSF or ZPL) and (Apache-2 or BSD)

@ryandesign
Copy link
Copy Markdown
Contributor

Yes (except "(Apache-2 or BSD)").

@mojca
Copy link
Copy Markdown
Member

mojca commented Jul 19, 2020

Yes (except "(Apache-2 or BSD)").

Oh, sorry, yes (question fixed).
We probably need an unit test for this inside the webapp.

@arjunsalyan arjunsalyan merged commit adba67d into macports:master Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Format of the License field

4 participants