CommunityBridge Project by Yash Varshney #148

Yash-Varshney · 2020-10-07T20:05:00Z

Hello Everyone, I am Yash Varshney and this P.R. is regarding the community bridge mentorship programme. This pr contains all the four task done by me, previously I have made the p.r. for two tasks but this one contains all the task together. This project updates the python tool with v2.2 (although not completely) and introduces a CLI tool for parser and convertor. I have and will be raising some issues regarding future work needed to do. The next task will be to update the python tool with v3.x of python.
It was a great learning experience for me for the last 3 months and I will further be working with spdx-community.
This P.R. is open for review.

Passing all the tests.
Review needed.

Signed-off-by: Yash Varshney b18038@students.iitmandi.ac.in

…sh Varshney as a part of CommunityBridgeMentorship Programme. The main feature introduce is the CLI-tool for python parser and convertor. Apart from this, Relationship Class is added in python tool, all the other classes have been updated wrt to v2.2 of spdx specs (some issues have also been raised), attribution text has been added to file,package and snippet class as well. BLACK is used for formatting the whole tool. Signed-off-by: Yash Varshney <b18038@students.iitmandi.ac.in>

Yash-Varshney · 2020-10-07T20:07:06Z

ping : @rtgdk @goneall

Yash-Varshney · 2020-10-07T20:08:40Z

Also raising this issue. A lot of points in this issue have been achieved but some are remaining, I will be raising issues for them ASAP.

goneall · 2020-10-07T22:23:32Z

@Yash-Varshney Thanks for the contributions and the PR!

@zvr, @rtgdk & @pombredanne - if you could review and let me know if your OK with me merging.

RishabhBhatnagar

Overall, good work. Thank You for contributing to the SPDX community.
Although, I had a hard time scanning the significant changes because of petty format changes like changing single-quote to double-quote, formatting spaces, adding lines, etcetera.

If possible, segregate changes into two commits. One for significant changes and the other for formatting.

RishabhBhatnagar · 2020-10-08T07:57:40Z

examples/parse_json.py

-if __name__ == '__main__':
-    import sys
+
+def parse_JSON(file):


Since python is a dynamically-typed language, it is harder to determine what should be the type of input argument.
In my perspective, the file variable might be a file object.

But upon seeing the usage of the variable file, seems like it is a path string.
Either the variable should be renamed for unambiguity or docstring should specify the data-type of the file variable.

Thanks for your feedback Rishabh, I feel that file variable does more justice as it literally means the address of the path where the file is.

examples/parse_json.py

RishabhBhatnagar · 2020-10-08T08:25:47Z

examples/parse_json.py

+if __name__ == "__main__":
+    import sys
+
+    file = sys.argv[1]


This will raise an IndexError if the filename is not provided in the cli input.
It should rather inform the user about the usage when the filename is not provided while running the script.

Do these changes for other example files as well.

@RishabhBhatnagar this parse_json (and all other files) are used for the cli_tool only. It actually doesn't make sense to use them without input files.
Moreover, due to the introduction of cli_tool, these files won't be used .
Thanks

No user will make an error consciously. If he/she ends up not providing input, the program must terminate gracefully with an error message describing the usage.
Check out any of the examples in tools-golang/examples directory. Missing input describes the usage and terminates.
One more example: mv file_name when run in any unix-terminal will terminate stating "missing destination file".

Due to the introduction of cli_tool, which files won't be used?

Note: this is not a deal-breaker.

RishabhBhatnagar · 2020-10-08T08:41:06Z

spdx/cli_tools/convertor.py

+    elif outfile.endswith(".spdx"):
+        outfile_format = "tag"
+    elif outfile.endswith(".rdf.xml"):
+        outfile_format = "rdf"


Unreachable statement. This case is consumed by the case when the outfile ends in ".xml"

This statement is basically to show the support for rdf.xml file to a reader. Ofc, it is unreachable but it actually tells the format supported :)

subsuming case is when the outfile ends in ".xml".
If the outfile ends in ".xml", outfile_format is "xml"
If the outfile ends in ".rdf.xml", the expected outfile_format according to the code is "rdf" but the subsuming case will set the outfile_format to "xml".

This must be corrected.

RishabhBhatnagar · 2020-10-08T09:49:38Z

examples/parse_rdf.py

+            for relation in doc.relationships:
+                print("\tRelationship: {0}".format(relation.relationship))
+                try:
+                    print("\tRelationship: {0}".format(relation.comment))


AttributeError: Relationship object won't have any member called "comment"
Relationship.relationship_comment is the attribute you should use.

Moreover, rather than EAFP, you can use LBYL as the Relationship class exposes a function(has_comment) to check if a comment is set.

RishabhBhatnagar · 2020-10-08T10:29:45Z

spdx/cli_tools/convertor.py

+            "INPUT FILETYPE NOT SUPPORTED. (only RDF and TAG format supported)"
+        )
+
+    if outfile.endswith(".rdf"):


[Optional] compress the if elif else ladder by the number of outfile_formats we can have.

For example:

if outfile.endswith((".rdf", ".rdf.xml")): outfile_format = "rdf" elif outfile.endswith((".tag", ".spdx")): outfile_format = "tag" ....

RishabhBhatnagar · 2020-10-08T10:37:34Z

spdx/cli_tools/convertor.py

+
+def rdf_to_json(infile, outfile):
+    infile = str(infile)
+    outfile = str(outfile)


Redundant typecasts of infile and outfile to string. Input variables are already strings (Correct me if I am wrong).
This operation is performed in all the subsequent functions in this file.

input variables can be anything (mostly strings ofc) but as a sanity check I deliberately converted it to string because if somehow there is a file say 12345.spdx and the user wants to check it, it will break the code.

Even if the file name is "12345.spdx", the code won't break.

RishabhBhatnagar · 2020-10-08T10:52:13Z

spdx/cli_tools/convertor.py

+        print(os.strerror(e.errno))
+
+
+def rdf_to_tag(infile, outfile):


redundant functions with name *_to_* .

You're calling them via dynamic access to global variables. You can directly call the intended function rather than redirecting through a custom defined function.

For example:
instead of calling rdf_to_tag, you can directly call RDF_TO_TAG function.

This will reduce the script size significantly.

good point, thanks. It could be improved further :)

RishabhBhatnagar · 2020-10-08T11:18:06Z

spdx/relationship.py

+
+    @property
+    def relatedspdxelement(self):
+        return self.relationship.split(" ")[2]


Rather than properties splitting the member variables every time and returning nth part,

It is preferable if you set these properties in the constructor itself, as the members (relationship and relationship_comment) are static for an object.
This will also block creation of an invalid Relationship which don't have enough parts (refA, relationship_type and, refB).

RishabhBhatnagar · 2020-10-08T11:19:29Z

spdx/cli_tools/parser.py

+    To use : run `parser` using terminal or run `parser --file <file name>`
+
+    """
+    if file.endswith(".rdf"):


[Optional] This if-elif-else ladder can be compressed too.

rtgdk · 2020-10-19T12:19:55Z

@Yash-Varshney @goneall I tried it out. And it works fine. I couldn't review the whole code since it takes too much time to review the real changes.

@Yash-Varshney Can you please respond to the comments made by @RishabhBhatnagar regarding the code structure? Overall, things look okay to merge if we have approvals from main reviewers.

zvr · 2020-10-20T08:31:49Z

I'm also OK with merging this.
As I wrote in an email, I am not concerned with it being a single large PR: we had explicitly asked the author to work on his own branch for all of the three months and submit a final PR at the end. This is not an incremental small fix; it's a push of a large body of work implementing new functionality.

Of course, there are things that can/should be changed, but I propose we merge this and further work be done on the merged single base afterwards.

Yash-Varshney · 2020-10-21T08:55:27Z

Thanks, @rtgdk @zvr @goneall @RishabhBhatnagar for reviewing. Ik that due to formatting there are some redundant additions but it was needed to format the whole code once so that it won't be a problem in future.
There are some small changes suggested by @RishabhBhatnagar, but I also believe that the code is working fine and it should be merged first and then few additions can be made.
Looking forward, I am thinking of adding a test for checking the use of BLACK formatter.
Thank you :)

goneall · 2020-10-21T16:16:28Z

@Yash-Varshney The PR is showing that the branch is out of date with the base branch - can you update the branch and verify that it is still working.

@RishabhBhatnagar If your OK merging in its current state, can you create a separate issue for the outstanding issues and approve the PR?

RishabhBhatnagar · 2020-10-22T05:49:09Z

@RishabhBhatnagar If your OK merging in its current state, can you create a separate issue for the outstanding issues and approve the PR?

@goneall, If you say so, I'll approve the PR. But I am not in the favor of merging it as I couldn't run the convertor for any of the described ways in the readme file.

Anyways, my review won't affect the merging of this PR as I am not a maintainer for this repository.

goneall

Although there are some good suggestions from @RishabhBhatnagar still open, I have reviewed the code and tried out some of the functions which worked OK for me.

I'll go ahead and merge it.

goneall requested a review from pombredanne October 7, 2020 22:20

RishabhBhatnagar suggested changes Oct 8, 2020

View reviewed changes

Merge branch 'master' into task-3

efd0e59

RishabhBhatnagar approved these changes Oct 22, 2020

View reviewed changes

goneall approved these changes Oct 23, 2020

View reviewed changes

goneall merged commit d197a3a into spdx:master Oct 23, 2020

RishabhBhatnagar mentioned this pull request Oct 24, 2020

Convertor of cli_tool not working #152

Closed

jayvdb mentioned this pull request Nov 1, 2022

Moved the metadata into setup.cfg. #156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CommunityBridge Project by Yash Varshney #148

CommunityBridge Project by Yash Varshney #148

Yash-Varshney commented Oct 7, 2020 •

edited

Loading

Yash-Varshney commented Oct 7, 2020

Yash-Varshney commented Oct 7, 2020

goneall commented Oct 7, 2020

RishabhBhatnagar left a comment

RishabhBhatnagar Oct 8, 2020

Yash-Varshney Oct 21, 2020

RishabhBhatnagar Oct 8, 2020

Yash-Varshney Oct 21, 2020

RishabhBhatnagar Oct 21, 2020

RishabhBhatnagar Oct 8, 2020

Yash-Varshney Oct 21, 2020

RishabhBhatnagar Oct 21, 2020

RishabhBhatnagar Oct 8, 2020

RishabhBhatnagar Oct 8, 2020

RishabhBhatnagar Oct 8, 2020

Yash-Varshney Oct 21, 2020

RishabhBhatnagar Oct 21, 2020

RishabhBhatnagar Oct 8, 2020

Yash-Varshney Oct 21, 2020

RishabhBhatnagar Oct 8, 2020

RishabhBhatnagar Oct 8, 2020

rtgdk commented Oct 19, 2020

zvr commented Oct 20, 2020

Yash-Varshney commented Oct 21, 2020

goneall commented Oct 21, 2020

RishabhBhatnagar commented Oct 22, 2020

goneall left a comment

CommunityBridge Project by Yash Varshney #148

CommunityBridge Project by Yash Varshney #148

Conversation

Yash-Varshney commented Oct 7, 2020 • edited Loading

Yash-Varshney commented Oct 7, 2020

Yash-Varshney commented Oct 7, 2020

goneall commented Oct 7, 2020

RishabhBhatnagar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtgdk commented Oct 19, 2020

zvr commented Oct 20, 2020

Yash-Varshney commented Oct 21, 2020

goneall commented Oct 21, 2020

RishabhBhatnagar commented Oct 22, 2020

goneall left a comment

Choose a reason for hiding this comment

Yash-Varshney commented Oct 7, 2020 •

edited

Loading