Skip to content

schema/ListedLicense: Do not allow <alt> or <optional> inside <alt> #475

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 8, 2017
Merged

schema/ListedLicense: Do not allow <alt> or <optional> inside <alt> #475

merged 1 commit into from
Nov 8, 2017

Conversation

wking
Copy link
Contributor

@wking wking commented Nov 1, 2017

By differentiating between formattedAltText…, which may contain <alt> and <optional>, and formattedFixedText…, which may not. The optionalType is using formattedAltTextGroup, because nesting variable content inside an optional element can be useful (#393, #462). The <alt> element, on the other hand, specifies all the variable options in its match attribute, so there's no need for nesting variable elements inside the <alt> body.

This nibbles off another non-contentious point from #462, and may allow implementation simplification without loss of generality. Depending on how #462 shakes out, we may transition optionalType from formattedAltTextGroup to formattedFixedTextGroup at a later date.

And there may be a DRYer XSD phrasing for these rules, but I'm not familiar enough with the syntax to work one up on short notice ;).

goneall
goneall reviewed Nov 3, 2017
View reviewed changes
schema/ListedLicense.xsd Outdated
@@ -59,7 +59,7 @@
</complexType>
<complexType name="notesType" mixed="true">
<choice minOccurs="1" maxOccurs="unbounded">
<element name="p" type="tns:formattedTextType"/>
<element name="p" type="tns:formattedAltTextType"/>
Copy link
Member

Choose a reason for hiding this comment

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

Notes does not support alternative or optional text - should this be formattedFixTextType? If we change it, will it cause a problem with the other elements named "p" in other elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we change it, will it cause a problem with the other elements named "p" in other elements?

I din't think so. Will rebase tonight.

goneall
goneall reviewed Nov 3, 2017
View reviewed changes
schema/ListedLicense.xsd Outdated
<element name="standardLicenseHeader" type="tns:formattedTextType" minOccurs="0" maxOccurs="1"/>
<element name="titleText" type="tns:formattedTextType" minOccurs="0" maxOccurs="1"/>
<element name="copyrightText" type="tns:formattedTextType" minOccurs="0" maxOccurs="1"/>
<element name="standardLicenseHeader" type="tns:formattedAltTextType" minOccurs="0" maxOccurs="1"/>
Copy link
Member

Choose a reason for hiding this comment

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

I believe we decided not to support optional and alt tags in the license header for this release. That being said, I could not find any record of that decision (gave up after a 10 minute search).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we decided not to support optional and alt tags in the license header for this release.

But thing like ISC have <alt> and <optional> and are the standard headers (although we don't say so yet, see #451). Is there a techical problem with them? Once we have alt/optional matching for the licenses, couldn't we use it for headers too?

Copy link
Member

Choose a reason for hiding this comment

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

@wking Are there any existing licenses using and/or in the standard headers? If not, we can implement this in the next release of the license list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any existing licenses using and/or in the standard headers?

Yup:

$ grep -rlz '<standardLicenseHeader>.*\(<alt \|<optional>\).*</standardLicenseHeader>' src | sort
src/AGPL-3.0.xml
src/Apache-2.0.xml
src/CPAL-1.0.xml
src/CUA-OPL-1.0.xml
src/ECL-1.0.xml
src/ECL-2.0.xml
src/GFDL-1.1.xml
src/GFDL-1.2.xml
src/GFDL-1.3.xml
src/GPL-1.0.xml
src/GPL-2.0.xml
src/GPL-3.0.xml
src/Interbase-1.0.xml
src/LGPL-2.0.xml
src/LGPL-2.1.xml
src/LPPL-1.0.xml
src/LPPL-1.1.xml
src/LPPL-1.2.xml
src/LPPL-1.3a.xml
src/LPPL-1.3c.xml
src/MPL-1.0.xml
src/MPL-1.1.xml
src/OCLC-2.0.xml
src/RPSL-1.0.xml
src/SGI-B-1.0.xml
src/SGI-B-1.1.xml
src/SISSL-1.2.xml
src/SISSL.xml
src/SPL-1.0.xml
src/W3C-20150513.xml
src/W3C.xml

Copy link
Member

Choose a reason for hiding this comment

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

OK - sounds like we should support the markup in this release. I logged 2 separate issues for the SPDX tools to support templates in the standard license header. I do not think either of these issues are critical to releasing this version of the license list, so we can proceed with keeping this in the schema.

Issues logged:

@goneall
Copy link
Member

goneall commented Nov 3, 2017

Is there a techical problem with them?

No technical issue - just a SMOP (simple matter of programming).

I just added support for templates, it took about a day (assuming I didn't introduce any bugs ;)

@wking
Copy link
Contributor Author

wking commented Nov 5, 2017

So is there anything else that needs to happen in this PR, or is it good to go?

@goneall
Copy link
Member

goneall commented Nov 5, 2017

@wking Have you addressed comment on the notes above (not supporting alt types for notes)?

@wking wking mentioned this pull request Nov 5, 2017
Closed
@wking
Copy link
Contributor Author

wking commented Nov 5, 2017

Have you addressed comment on the notes above (not supporting alt types for notes)?

Ah, right. I still need to do that.

@wking wking force-pushed the no-variables-inside-alt branch from a1f48eb to 1825366 Compare November 6, 2017 20:17
@wking
Copy link
Contributor Author

wking commented Nov 6, 2017

Ok, I've made <notes> fixed with a1f48eb → 1825366. This also drops the unused notesType.

I'm pretty sure we don't want titleText or copyrightText in the choices for either formatted*TextGroup, but I'm leaving that for #452.

@wking
schema/ListedLicense: Do not allow <alt> or <optional> inside <alt>
1825366 
By differentiating between formattedAltText..., which may contain
<alt> and <optional>, and formattedFixedText..., which may not.  The
optionalType is using formattedAltTextGroup, because nesting variable
content inside an optional element can be useful [1,2].  The <alt>
element, on the other hand, specifies all the variable options in its
match attribute, so there's no need for nesting variable elements
inside the alt body.

Also require fixed text for <notes>, since those aren't templates.
And drop the unused notesType, since they're vanilla formatted text.

We almost certainly want to drop titleText and copyrightText fro the
formatted*TextGroup choices, but I'm leaving that for [3].

[1]: spdx#393
[2]: spdx#462
[3]: spdx#452
@goneall
Copy link
Member

goneall commented Nov 6, 2017

Changes look good to me. I'll test out the schema using the license generator tool before merging. It may take a couple of days since I am in the middle of updating the tools to support template functionality in the standard license header.

@goneall goneall merged commit ce845e1 into spdx:master Nov 8, 2017
@wking wking deleted the no-variables-inside-alt branch November 8, 2017 05:00
@wking wking mentioned this pull request Nov 8, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goneall goneall goneall left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wking @goneall