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

Generated documentation for ExternalRef shows superclass of itself #960

Open
goneall opened this issue Jan 22, 2025 · 19 comments
Open

Generated documentation for ExternalRef shows superclass of itself #960

goneall opened this issue Jan 22, 2025 · 19 comments
Labels
documentation Improvements or additions to documentation
Milestone

Comments

@goneall
Copy link
Member

goneall commented Jan 22, 2025

ExternalRef should not have any superclass, bit it shows itself as a superclass in the generated documentation.

I'm not sure if this is an issue with the model, SpecParser of MkDocs in the spec - so I thought I would post this here to start.

See https://spdx.github.io/spdx-spec/v3.0.1/model/Core/Classes/ExternalRef/

@goneall goneall added the documentation Improvements or additions to documentation label Jan 22, 2025
@goneall goneall added this to the 3.1 milestone Jan 22, 2025
@bact
Copy link
Collaborator

bact commented Jan 22, 2025

In the Metadata section of the model Markdown file, it looks fine (SubclassOf: none):

## Metadata
- name: ExternalRef
- SubclassOf: none
- Instantiability: Concrete

@bact
Copy link
Collaborator

bact commented Jan 22, 2025

We may need to target this as 3.0.2 ?
(Or even 3.0.1 ? - since this is not a change to the model itself, but the rendering/generation of the model documentation?)

@bact
Copy link
Collaborator

bact commented Jan 22, 2025

This probably more of a layout/formatting/choice of word issue.

There are also other classes that have the same issue, like:
https://spdx.github.io/spdx-spec/v3.0.1/model/Core/Classes/IntegrityMethod/

Image

https://spdx.github.io/spdx-spec/v3.0.1/model/Core/Classes/Hash/

Image

Probably have to have some indication that, the class at the top is not the superclass but "this class".

@bact
Copy link
Collaborator

bact commented Jan 22, 2025

Relevant code block in spec-parser:

## Superclasses

{% set full_inheritance_stack = [fqname] + inheritance_stack %}
{% for super in full_inheritance_stack | reverse %}
{{ ' ' * ((loop.index-1) * 6) }}
  {% if loop.last %}
{{type_link(super)}}
  {% else %}
{{type_link(super)}}<br />
  {% endif %}
{% endfor %}

https://github.com/spdx/spec-parser/blob/3d1d92eb1ccd95f65d3cc54a7d6837b32cf6aeab/spec_parser/templates/mkdocs/class.md.j2#L13-L23

@zvr
Copy link
Member

zvr commented Jan 22, 2025

Right -- it's the chain of classes ending at the class in question (of that page).

@goneall
Copy link
Member Author

goneall commented Jan 22, 2025

Is the solution to remove the SubclassOf: none or update the spec-parser to handle this case?

@zvr
Copy link
Member

zvr commented Jan 22, 2025

spec-parser can generate what is needed.

But what is the desired outcome?

What should be generated in cases where:

  • there are no superclasses (e.g. Element)
  • there is only one superclass (e.g. Artifact)
  • there are more than one (e.g. File)

?

Right now, in all cases, what is being generated is the complete list of classes, ending with (and including) the class in question.

@zvr
Copy link
Member

zvr commented Jan 22, 2025

Adding more context, the output has always been like this since we added this functionality 5 months ago.

The way to think about what is presented is a "class hierarchy", if you will, showing "inheritance". But in our spec, we are defining RDF classes, and the words "inheritance" and "hierarchy" are not acceptable in that domain; RDF talks about "superclasses" only.

@zvr
Copy link
Member

zvr commented Jan 26, 2025

@goneall any additional input on how you want to see this?

I see you put the 3.1 milestone; are you OK leaving this as it currently is in the PDF to go to ISO? Or do you want to have it reworked / reworded?

Would a section title like "List of classes" or something similar be better?

@goneall
Copy link
Member Author

goneall commented Jan 26, 2025

What should be generated in cases where:

there are no superclasses (e.g. Element)

My current opinion is just using something like "None" to indicate there is no superclass

Alternatives I'm also OK with:

  • We could not include the superclass section
  • If we want to be RDF specific - we could use owl:Thing as the superclass

there is only one superclass (e.g. Artifact)

I would just have the superclasses - not the class itself (e.g. just Element for Artifact)

there are more than one (e.g. File)

I would include all the hierarchy except the class itself

@goneall
Copy link
Member Author

goneall commented Jan 26, 2025

@goneall any additional input on how you want to see this?

I see you put the 3.1 milestone; are you OK leaving this as it currently is in the PDF to go to ISO? Or do you want to have it reworked / reworded?

I'm OK with fixing this later in 3.1 - I consider it mostly a documentation issue. If someone complains in the ISO review, we can fix it on an update.

Would a section title like "List of classes" or something similar be better?

Now that I think about it, we could just rename the title "Class Hierarchy", then having the class itself in the section would make sense (at least to me) since you can have the class in the hierarchy but it technically isn't a superclass of itself. If we rename it, I wouldn't change anything else.

If we don't include the class itself in the section (as proposed above), the current title is fine.

@zvr
Copy link
Member

zvr commented Jan 27, 2025

"Class hierarchy" ?

@goneall
Copy link
Member Author

goneall commented Jan 28, 2025

From tech call discussion on 28 Jan 2025: Sean would prefer not to display the "Superclasses" if there are no superclasses, but he's fine with the proposal to just rename to "Class hierarchy".

Perhaps we just rename to "Class hierarchy" since it is the easiest approach.

Also note: we discussed a longer term solution where Element is a subclass of "xxx:Thing" and the non-elements would subclass something else that we define as being used as a property of an element - not something intended to be standalone.

@sbarnum
Copy link
Collaborator

sbarnum commented Jan 28, 2025

Just to elaborate on Gary's comment above from my suggestion.
We faced this exact issue in the UCO effort and ended up defining two new classes (UcoThing and UcoInherentCharacterizationThing) to help define the difference between domain object classes (Elements in SPDX parlance) and non-domain object classes that are actually characterizations of some aspect of domain object classes (Non-Element classes in SPDX parlance).
Long term, I would suggest specifying a new "SPDXThing" (or something similar) class and making Element a subclass of it, and also specifying a new "SPDXInherentCharacterizationThing" (or something similar) class and making all of the non-element classes in the model core diagram subclasses of it. And lastly, define the two new classes as disjoint.

For now, I support changing the title of the "Superclasses" field of the generated docs to "Class Hierarchy". :-)

@zvr
Copy link
Member

zvr commented Jan 30, 2025

The change of the title is already done, thank you all!

To the suggestions by @sbarnum , I want to note:

  • we already have a single "root" for all SPDX-related classes and this is Element. I think this is effectively the same as "SPDXThing" mentioned, and I do not find it necessary to create another class which will only have Element as a subclass.
  • I completely agree with creating another superclass for all non-Element classes. In our original diagrams the box on the right was named "Dataclasses", IIRC, so we could use SPDXDataclass. And we can say that this class and Element are disjoint.

@bact
Copy link
Collaborator

bact commented Jan 31, 2025

Thank you.

Do we like to regenerating https://spdx.github.io/spdx-spec/v3.0.1/ with this new heading name or;
do we like to really freeze v3.0.1 content (even the heading is not part of the model) and wait for post-v3.0.1 ?

@goneall
Copy link
Member Author

goneall commented Jan 31, 2025

IMHO we don't need to do a new spec release for a change in the heading. It doesn't change anything in the spec or model repo, so adding a tag would not include any content. Although, we may want to consider producing a new PDF for OMG and ISO.

@bact
Copy link
Collaborator

bact commented Jan 31, 2025

IMHO we don't need to do a new spec release for a change in the heading. It doesn't change anything in the spec or model repo, so adding a tag would not include any content. Although, we may want to consider producing a new PDF for OMG and ISO.

If we rerun the publish CI with the new spec-parser, it will have the new heading.

If we don't see that's an issue, we can just let it naturally occurs
(For example, in the next run after we have a translation ---- note that we need to inform the translation teams about this new heading too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

4 participants