Skip to content

[issue-246] parse SPDXID for file correctly - #247

Closed
meretp wants to merge 2 commits into
spdx:mainfrom
meretp:issue_246
Closed

meretp wants to merge 2 commits into
spdx:mainfrom
meretp:issue_246

Conversation

@meretp

@meretp meretp commented Oct 20, 2022

Copy link
Copy Markdown
Collaborator

fixes #246

Signed-off-by: Meret Behrens meret.behrens@tngtech.com

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
Comment thread spdx/parsers/tagvaluebuilders.py Outdated
if self.has_package(doc) and self.has_file(doc):
if not self.file_spdx_id_set:
self.file_spdx_id_set = True
spdx_id = spdx_id.split("#")[-1]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you plan to disregard the part before #, you have to at least check that this is exactly the same as the current enclosing identifier.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your review. I don't think that the validation should be part of this PR. In the current implementation this isn't checked for any other ID, e.g. for the package SPDX ID. I would suggest to open another issue for this validation so that the validation is consistent for all SPDXIDs parsed from a rdf-file. Are you okay with that?
Nevertheless I see the point to transfer this rdf-specific logic to the rdf-builder. I will change this.

@zvr zvr Oct 22, 2022 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Moving it to RDF processing is correct, thanks for that.

I still maintain that the namespace should not be blindly discarded.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I opened an issue concerning the validation here #251. It think it makes sense to introduce the valdiation for all values in one PR. Are you okay with that and would you approve this PR with that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine with handling the namespace elsewhere, especially if it happens for all cases.

But as I wrote in #251, it's not a validation. It's a check of whether it can be discarded. In some cases (when it is different than the enclosing namespace), it will have to be retained.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I see. I didn't know that there is a difference. But this does also apply to the package SPDX ID, right, e.g. here? I will update the open issue.
With that I think it is best to close this PR without merging and I will change the implementation in #254 so that it is independant from this problem.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it might happen in any id you encounter, e.g., document id, snippet id, license id, ...

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
meretp pushed a commit to meretp/tools-python that referenced this pull request Oct 26, 2022
…spec 2.2

leads to failing test, which should be solved once PR spdx#247 is merged
Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
meretp pushed a commit to meretp/tools-python that referenced this pull request Oct 26, 2022
…spec 2.2

leads to failing test, which should be solved once PR spdx#247 is merged
Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
@meretp meretp added the parser Issues related to the parser layer label Oct 27, 2022
@meretp meretp closed this Oct 27, 2022
@meretp
meretp deleted the issue_246 branch March 2, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parser Issues related to the parser layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SPDXID from file incorrectly parsed by rdf parser

2 participants