Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FsfLicenseDataParser: Fill in implementation #112

Closed
wants to merge 1 commit into from

Conversation

wking
Copy link
Contributor

@wking wking commented Oct 25, 2017

Pull the index (mapping from SPDX IDs to FSF IDs) during initialization, and then lookup each license as needed with a separate request.

An implementation that hits isSpdxLicenseFsfLibre multiple times (or which wanted to extract multiple values for a single license) would be more efficient if we cached the per-license FSF response. I've left that off for now because our only consumer is just asking for libre-ness, and only doing that once per license.

I've used a Boolean for the isSpdxLicenseFsfLibre to get a nullable value, so we can represent:

  • “yes, the FSF marks that license ‘libre’” (true),
  • “no, the FSF considers that license non-free” (false), and
  • “we don't know the FSF opinion for that license” (null).

Related discussion in wking/fsf-api#1 and wking/fsf-api#5.

And this may be my first Java ever, so please review carefully and don't assume I know what I'm doing ;).

Pull the index (mapping from SPDX IDs to FSF IDs) during
initialization, and then lookup each license as needed with a separate
request.

An implementation that hits isSpdxLicenseFsfLibre multiple times (or
which wanted to extract multiple values for a single license) would be
more efficient if we cached the per-license FSF response.  I've left
that off for now because our only consumer is just asking for
libre-ness, and only doing that once per license.

I've used a Boolean for the isSpdxLicenseFsfLibre to get a nullable
value, so we can represent:

* "yes, the FSF marks that license 'libre'" (true),
* "no, the FSF considers that license non-free" (false), and
* "we don't know the FSF opinion for that license" (null).
@goneall
Copy link
Member

goneall commented Oct 26, 2017

@wking After reviewing the pull request, we are going to do a file open on every isFsfLibre request which introduces performance and additional possibilities of errors. I also have implemented features in the SPDX tools to work offline, so we would need to add an offline cache of the FSF API. This is much easier to implement and keep in sync as a single file.

I understand the desire to have a "clean" implementation of an API, but the same argument could even be made for storing the SPDX ID. That being said, my opinion is that we should include the flag in the index for practical reasons.

Can you describe any practical issues with including the flag in the index (e.g. performance or maintenance issues)? And do you believe these issues outweigh the practial issues with not including the information in the index?

There are several examples of indexes and file system directory structures where metadata is stored in the index itself.

One other consideration, the FsfLibre is a core piece of FSF information and, in my opinion, the main reason for the FSF list. Therefore it should be included in the index.

Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

I would like the libre flag to be cached, ideally populated in the constructor so we don't have to open a file on every flag request.

We would also need to support the tool running offline. This can be done by copying a copy of the JSON files to the resources folder and accessing the files if the website is not available (see the ListedLicenses as an example).

@wking
Copy link
Contributor Author

wking commented Oct 26, 2017 via email

@goneall
Copy link
Member

goneall commented Oct 26, 2017

Can we have this discussion in wking/fsf-api#1, where it will be
separate from this particular implementation?

Moved conversation to wking/fsf-api#1

@goneall
Copy link
Member

goneall commented Nov 11, 2017

@wking Thanks for the implementation, but I decided to implement using a single JSON file for reliability reasons. I have tried to implement the SPDX tools in a way that it did could work offline which required a cache of the JSON information. The changes are in this commit: 3718ca1

@goneall goneall closed this Nov 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants