Skip to content

Add Copyright class and copyright_diff() #82 #83 #85

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

Merged
steven-esser merged 7 commits into from
Mar 8, 2018
Merged

Add Copyright class and copyright_diff() #82 #83 #85

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
00b6e7a
Add 'Copyright' class #82
johnmhoran Feb 26, 2018
f8f5861
Add copyright_diff() to DeltaCode object #83
johnmhoran Mar 1, 2018
f1c9f4d
Add additional copyright-related tests #83
johnmhoran Mar 2, 2018
59b502d
Refactor copyright-related code #82 #83
johnmhoran Mar 6, 2018
d2cb79d
Rename method to 'delta.update()' #82 #83
johnmhoran Mar 6, 2018
195eb00
Test copyrights for unusual characters #82 #83
johnmhoran Mar 7, 2018
4199bcd
Remove print statements #82 #83
johnmhoran Mar 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 18 additions & 19 deletions src/deltacode/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,21 +199,18 @@ def license_diff(self):
old_licenses = delta.old_file.licenses or []

if len(delta.new_file.licenses) > 0 and delta.old_file.licenses == []:
delta.factors.append('license info added')
delta.score += 20
delta.add_score(20, 'license info added')

@steven-esser steven-esser Mar 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs to be renamed to something like delta.update.
We are not only adding to the score, we are also adding to factors etc and possibly more in the future.

return

if delta.new_file.licenses == [] and len(delta.old_file.licenses) > 0:
delta.factors.append('license info removed')
delta.score += 15
delta.add_score(15, 'license info removed')
return

new_keys = set(license.key for license in new_licenses)
old_keys = set(license.key for license in old_licenses)

if new_keys != old_keys:
delta.factors.append('license change')
delta.score += 10
delta.add_score(10, 'license change')

def copyright_diff(self):
"""
Expand All @@ -231,13 +228,10 @@ def copyright_diff(self):
old_copyrights = delta.old_file.copyrights or []

if len(delta.new_file.copyrights) > 0 and delta.old_file.copyrights == []:
delta.factors.append('copyright info added')
delta.score += 15
delta.add_score(10, 'copyright info added')
return

if delta.new_file.copyrights == [] and len(delta.old_file.copyrights) > 0:
delta.factors.append('copyright info removed')
delta.score += 10
elif delta.new_file.copyrights == [] and len(delta.old_file.copyrights) > 0:
delta.add_score(10, 'copyright info removed')
return

@steven-esser steven-esser Mar 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably best that we score these the same. This will also allow for you to combine the logic into a single if statement


new_statements = set(statement for copyright in new_copyrights for statement in copyright.statements)
Expand All @@ -246,14 +240,9 @@ def copyright_diff(self):
new_holders = set(holder for copyright in new_copyrights for holder in copyright.holders)
old_holders = set(holder for copyright in old_copyrights for holder in copyright.holders)

new_authors = set(author for copyright in new_copyrights for author in copyright.authors)
old_authors = set(author for copyright in old_copyrights for author in copyright.authors)

if ((new_statements != old_statements) or
(new_holders != old_holders) or
(new_authors != old_authors)):
delta.factors.append('copyright change')
delta.score += 5
(new_holders != old_holders)):
delta.add_score(5, 'copyright change')

def index_deltas(self, index_key='path', delta_list=[]):
"""
Expand Down Expand Up @@ -291,6 +280,16 @@ def __init__(self, score=0, new_file=None, old_file=None):
self.factors = []
self.score = score

def add_score(self, score=0, factor=''):

@steven-esser steven-esser Mar 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

like stated above, this needs to be renamed to update.

"""
For each Delta object identified in DeltaCode.license_diff() or
DeltaCode.copyright_diff(), add the score to the object's 'score'

@steven-esser steven-esser Mar 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We do not need ot mention these funtions here. Simply tell me what this update function does.

attribute and add a string, summarizing the factor associated with the
score, to the object's 'factors' attribute (a list).
"""
self.factors.append(factor)
self.score += score

def to_dict(self):
"""
Return an OrderedDict comprising the 'factors', 'score' and new and old
Expand Down
4 changes: 1 addition & 3 deletions src/deltacode/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,6 @@ class Copyright(object):
def __init__(self, dictionary={}):
self.statements = dictionary.get('statements')

@steven-esser steven-esser Mar 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you haven’t already, you should add tests cases where a files have large numbers of copyrights holders and statements to see if we choke somewhere, espically in the output

@steven-esser steven-esser Mar 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also tests where there are strange characters or accent marks in the copyright statements/holders.

All of these additional test cases I mentioned should probably come from scancode generated output directly as opposed to crafting the test object by hand.

@johnmhoran johnmhoran Mar 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@MaJuRG Since we want to use ScanCode-generated output, do you have any codebases in mind that satisfy the characteristics you describe?

I've started to work my way through the codebases we've worked with (openssl, zlib et al.) but I've not yet seen large numbers of copyright holders/statements or unusual characters.

@steven-esser steven-esser Mar 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@johnmhoran You will probably just have to hand-create a file or files that contain a bunch of copyright statements

@johnmhoran johnmhoran Mar 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK. Thanks, @MaJuRG .

@johnmhoran johnmhoran Mar 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@MaJuRG I'm finding that an error is thrown when I include a French accent character in the copyright statements or holders value, e.g., "é". The error is thrown even if I comment out the character (presumably because even comments are parsed). Unicode and UTF-8 do not throw an error.

...
"statements": [
    "U+00E9",
    "\xc3\xa9"
    # "é"
...

SyntaxError: Non-ASCII character '\xc3' in file C:\code\nexb\dev\deltacode\tests\test_models.py on line 1073, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

While PEP 263 gives some suggestions, it's not clear to me how we can apply these to handle our input. I've done some searching in the ScanCode repo -- surely ScanCode must be able to handle such characters -- but have not yet found how ScanCode addresses this.

@johnmhoran johnmhoran Mar 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looks like this issue might be addressed in scancode-toolkit/src/commoncode/text.py?

@steven-esser steven-esser Mar 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@johnmhoran where is this "statements" located?

@johnmhoran johnmhoran Mar 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@MaJuRG Per your suggestion above, I'm hand-crafting old and new files in a new test, test_Copyright_unusual_characters(), in test_models.py.

@johnmhoran johnmhoran Mar 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@MaJuRG Would it be easier for you if I commit and push? Except for this one failing test, it's ready for your review.

self.holders = dictionary.get('holders')
self.authors = dictionary.get('authors')

@steven-esser steven-esser Mar 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Like I mentioned above, lets remove authors.

def to_dict(self):
"""
Expand All @@ -263,8 +262,7 @@ def to_dict(self):
"""
d = OrderedDict([
('statements', self.statements),
('holders', self.holders),
('authors', self.authors)
('holders', self.holders)
])

return d
Expand Down
2 changes: 1 addition & 1 deletion tests/data/cli/copyright_and_license_info_added.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
Factors,Score,Path,Name,Type,Size,Old Path
modified license info added copyright info added,55,path.txt,path.txt,file,300,
modified license info added copyright info added,50,path.txt,path.txt,file,300,
2 changes: 1 addition & 1 deletion tests/data/cli/copyright_info_added.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
Factors,Score,Path,Name,Type,Size,Old Path
modified copyright info added,35,path.txt,path.txt,file,300,
modified copyright info added,30,path.txt,path.txt,file,300,
2 changes: 1 addition & 1 deletion tests/data/cli/copyright_info_added_license_info_removed.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
Factors,Score,Path,Name,Type,Size,Old Path
modified license info removed copyright info added,50,path.txt,path.txt,file,300,
modified license info removed copyright info added,45,path.txt,path.txt,file,300,
78 changes: 36 additions & 42 deletions tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,13 +442,12 @@ def test_json_output_option_selected_all_selected(self):
],
"copyrights": [
{
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
],
"authors": []
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
]
}
]
},
Expand All @@ -470,13 +469,12 @@ def test_json_output_option_selected_all_selected(self):
],
"copyrights": [
{
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
],
"authors": []
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
]
}
]
}
Expand Down Expand Up @@ -507,13 +505,12 @@ def test_json_output_option_selected_all_selected(self):
],
"copyrights": [
{
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
],
"authors": []
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
]
}
]
},
Expand All @@ -535,13 +532,12 @@ def test_json_output_option_selected_all_selected(self):
],
"copyrights": [
{
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
],
"authors": []
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
]
}
]
}
Expand Down Expand Up @@ -600,13 +596,12 @@ def test_json_output_option_selected_all_not_selected(self):
],
"copyrights": [
{
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
],
"authors": []
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
]
}
]
},
Expand All @@ -628,13 +623,12 @@ def test_json_output_option_selected_all_not_selected(self):
],
"copyrights": [
{
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
],
"authors": []
"statements": [
"Copyright (c) 2017 Acme Software Inc. and others."
],
"holders": [
"Acme Software Inc. and others."
]
}
]
}
Expand Down
Loading