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

Assorted bug fixes #270

Merged
merged 5 commits into from
Nov 9, 2022
Merged

Assorted bug fixes #270

merged 5 commits into from
Nov 9, 2022

Conversation

meretp
Copy link
Collaborator

@meretp meretp commented Nov 9, 2022

Taken over from #263 resp. #207 because signoffs were missing.

This code is based on commits of the WhiteSource PS Team, esp. the authors: @tamari-oz, @NatalyaDalid, @rammatzkvosky and @DimarrWS. Thank you for your contribution!

@meretp meretp mentioned this pull request Nov 9, 2022
DimarrWS
DimarrWS previously approved these changes Nov 9, 2022
@nicoweidner nicoweidner mentioned this pull request Nov 9, 2022
Copy link
Collaborator

@nicoweidner nicoweidner left a comment

Choose a reason for hiding this comment

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

Thanks a lot for updating this! I just left a few minor comments

Comment on lines 17 to 18
# Implement the auto feature that becomes available in 3.6
autoinc = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it's redundant now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deleted

@@ -536,15 +537,30 @@ class RelationshipInfoWriter(BaseWriter):
def __init__(self, document, out):
super(RelationshipInfoWriter, self).__init__(document, out)

def create_relationship_node(self, relationship):
def create_relationshiptype(self, relationshiptype: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be relationship_type. Can you rename the properties on the Relationship class? Applies to spdxelementid and relatedspdxelement as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also:

  • the method name doesn't match what the method does anymore. Something like transform_relationship_type_to_rdf_model maybe? (but maybe you have a less clunky idea)
  • The logic looks a bit cryptic now. Maybe you can include an example in the docstring?
  • This is a conversion from upper snake case (hope that term exists...) to camel case, right? If so, we could mention that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored

type_triple = (relationship_node, RDF.type, self.spdx_namespace.Relationship)
self.graph.add(type_triple)

relationship_type_node = Literal(relationship.relationshiptype)
relationship_spdx_element = Literal(relationship.spdxelementid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

relationship_spdx_element_id

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

)
)
relationship_type = self.create_relationshiptype(relationship.relationshiptype)
relationship_type_node = self.spdx_namespace["relationshipType_" + relationship_type]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the method currently named create_relationshiptype retains the purpose of "converting to rdf-compatible style", I would include adding the relationshipType_ prefix in that method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

nicoweidner
nicoweidner previously approved these changes Nov 9, 2022
Copy link
Collaborator

@nicoweidner nicoweidner left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments!

This code is based on commits of the WhiteSource PS Team, esp. the authors: tamari-oz, NatalyaDalid, rammatzkvosky and DimarrWS. Thank you for your contribution!

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
This code is based on commits of the WhiteSource PS Team, esp. the authors: tamari-oz, NatalyaDalid, rammatzkvosky and DimarrWS. Thank you for your contribution!

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
This code is based on commits of the WhiteSource PS Team, esp. the authors: tamari-oz, NatalyaDalid, rammatzkvosky and DimarrWS. Thank you for your contribution!

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
This code is based on commits of the WhiteSource PS Team, esp. the authors: tamari-oz, NatalyaDalid, rammatzkvosky and DimarrWS. Thank you for your contribution!

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
This code is based on commits of the WhiteSource PS Team, esp. the authors: tamari-oz, NatalyaDalid, rammatzkvosky and DimarrWS. Thank you for your contribution!

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
Copy link
Collaborator

@nicoweidner nicoweidner left a comment

Choose a reason for hiding this comment

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

One more time after the force push

@meretp meretp merged commit 7e460b3 into spdx:main Nov 9, 2022
@meretp meretp deleted the assorted_bug_fixes branch November 9, 2022 15:04
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.

3 participants