bears/general: Add LicenseCheckBear #1442

yash-nisar · 2017-02-18T07:00:47Z

Closes #823

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

I read the commit guidelines and I've followed
them.
I ran coala over my code locally. (All commits have to pass
individually. It is not sufficient to have "fixup commits" on your PR,
our bot will still report the issues for the previous commit.) You will
likely receive a lot of bot comments and build failures if coala does not
pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

pick up any PR at https://coala.io/review
review it (check https://coala.io/reviewing for more info)
if you are sure that it needs work, use cobot mark wip <URL> to get it out
of the review queue.

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

yash-nisar · 2017-02-18T19:54:00Z

@Makman2 Appveyor build failing. However the circle ci and travis builds have passed. I have added the dependencies in .travis.yml and deps.sh. What could be the reason ?

yash-nisar · 2017-02-18T22:31:05Z

@Makman2 @adtac Please have a look at this when you are free. :)

adtac · 2017-02-19T05:50:23Z

bears/general/LicenseCheckBear.py

+        """
+        args = ()
+        if licensecheck_lines != 60:
+            args += ('--lines', str(licensecheck_lines))


Just give this directly, there's no need for a conditional statement.

adtac · 2017-02-19T05:50:48Z

bears/general/LicenseCheckBear.py

+
+    @staticmethod
+    def create_arguments(filename, file, config_file,
+                         licensecheck_lines: int = 60,


No spaces around the = (see also the other lines below)

adtac · 2017-02-19T05:51:54Z

tests/general/LicenseCheckBearTest.py

@@ -0,0 +1,78 @@
+from queue import Queue
+import os.path


We usually just do import os :)

Any particular reasons for import os.path?

Idk why I imported os.path 😆

adtac · 2017-02-19T05:54:55Z

tests/general/LicenseCheckBearTest.py

+    def test_license(self):
+        file_contents = [load_testfile(
+            get_testfile_path('apache_license_2.py'))]
+        test_file_path = get_testfile_path('apache_license_2.py')


move this one line above and reuse test_file_path

adtac · 2017-02-19T05:56:41Z

tests/general/LicenseCheckBearTest.py

+            self.uut,
+            file_contents,
+            [Result.from_values('LicenseCheckBear',
+                                test_file_path + ': Apache (v2.0)',


Wait, this will return a result all the time? I thought this bear is something like "Make sure all files have this particular license; if not, return a result" and that it'll be quiet if a file is okay?

Or is this a helper bear for other bears?

Unlike other bears that check validity,

This bear attempts to determine the license that applies to each file passed to
it, by searching the start of the file for text belonging to various
licenses.

It will return a result all the time to determine not just the license but also the type of license.

this is then definitely suited for a dependency bear imo, and we should add a bear that uses that information. Like a LicenseConsistencyBear or so^^

I'd never enable a bear that always returns results - it'll just clutter the output window. This should definitely be a dependency bear. And in that case, you should use HiddenResults instead of Results.

In the case of a native bear, I can silence the results in the run method but how do I silence the results given by the licensecheck tool in this case ? @adtac @Makman2

@linter currently doesn't support yielding HiddenResults currently, you maybe want to implement support for it^^

So, till the time yielding HiddenResult is implemented for linter bears, what do we do with this ?

yash-nisar · 2017-02-19T06:29:49Z

Done @adtac

sils · 2017-02-20T08:35:30Z

bears/general/LicenseCheckBear.py

+        :param licensecheck_check:
+            Specify a pattern indicating which files should be checked.
+        :param licensecheck_copyright:
+            Display the file's copyright


I don't see what this is good for or even what it does

We were actually planning to write a bear that would check for copyright notices in source files as mentioned here. However we have wrapped this functionality in a tool called as licensecheck which determines the license that applies to each file and checks for copyright notices too (displays it if present).

sils · 2017-02-20T08:35:42Z

bears/general/LicenseCheckBear.py

+            Specify that files/directories matching the regular expression
+            should be ignored when checking files.
+        :param licensecheck_check:
+            Specify a pattern indicating which files should be checked.


can you add examples? Solely from this description I'd not know how to use it

We can remove this parameter too IMO because the user can explicitly specify it either by the --files argument or in the .coafile with the help of globs

sils · 2017-02-20T08:36:14Z

bears/general/LicenseCheckBear.py

+            for license information.
+        :param licensecheck_ignore:
+            Specify that files/directories matching the regular expression
+            should be ignored when checking files.


this shouldn't be given, coala does ignoring and we shouldn't repeat the functionality as a setting

gitmate-bot · 2017-02-23T20:24:28Z

bears/general/LicenseCheckBear.py

+        args += ('--lines', str(licensecheck_lines))
+        if licensecheck_copyright:
+            args += ('--copyright',)
+        return args + (filename,)


W292 no newline at end of file'

PycodestyleBear (W292), severity NORMAL, section autopep8.

gitmate-bot · 2017-02-23T20:24:29Z

bears/general/LicenseCheckBear.py

+        args += ('--lines', str(licensecheck_lines))
+        if licensecheck_copyright:
+            args += ('--copyright',)
+        return args + (filename,)


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/bears/general/LicenseCheckBear.py +++ b/bears/general/LicenseCheckBear.py @@ -34,4 +34,4 @@ args += ('--lines', str(licensecheck_lines)) if licensecheck_copyright: args += ('--copyright',) - return args + (filename,)+ return args + (filename,)

yash-nisar · 2017-03-06T20:00:56Z

Surprisingly, everything is green again. Over to you, @Makman2 @jayvdb.

Makman2 · 2017-03-06T21:19:55Z

bears/general/LicenseCheckBear.py

+
+@linter(executable='licensecheck',
+        output_format='regex',
+        output_regex=r'(?P<filename>.*): (?P<copyright>.*)UNKNOWN',


unknown capturing groups, please remove them as coala will otherwise raise a warning on execution^^

Makman2 · 2017-03-06T21:21:44Z

bears/general/LicenseCheckBear.py

+class LicenseCheckBear:
+    """
+    Attempts to check the given file for a license, by searching the start
+    of the file for text belonging to various licenses. For Ubuntu/Debian


Would layout the docs like this:

Attempts to check the given file for a license, by searching the start of the file for text belonging to various licenses. For Ubuntu/Debian ...

As the Ubuntu/Debian part is detailed :) Rule of thumb: The first line/sentence is the short description which shall describe the bear as short as possible, everything following an empty line is detailed description :)

Makman2 · 2017-03-06T21:22:09Z

bears/general/LicenseCheckBear.py

+    Attempts to check the given file for a license, by searching the start
+    of the file for text belonging to various licenses. For Ubuntu/Debian
+    users, the licensecheck_lines option has to be used in accordance with the
+    licensecheck_tail option.


Enclose licensecheck_lines and licensecheck_tail in double backticks :)

Makman2 · 2017-03-06T21:23:36Z

bears/general/LicenseCheckBear.py

+        :param licensecheck_lines:
+            Specify how many lines of the file header should be parsed
+            for license information. Set to 0 to parse the whole
+            file (and ignore licensecheck_tail).


Enclose licensecheck_tail in double backticks

Makman2 · 2017-03-06T21:24:16Z

bears/general/LicenseCheckBear.py

+            file (and ignore licensecheck_tail).
+        :param licensecheck_tail:
+            Specify how many bytes to parse at end of file. Set to 0 to
+            avoid parsing from end of file.


Would rephrase to

Set to 0 to disable parsing from end of file.

Makman2 · 2017-03-06T21:24:54Z

bears/general/LicenseCheckBear.py

+            avoid parsing from end of file.
+        """
+        return '--lines', str(licensecheck_lines), '--tail', \
+               str(licensecheck_tail), filename


use braces instead of \:

return ('--lines', ..., ..., 'folllowing-args', ...)

Makman2 · 2017-03-06T21:25:28Z

tests/general/licensecheck_test_files/mit_license.py

+# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+# LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+# OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+# WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.


what about adding some code to these files?

(to some of them)

Added some code too and update the regex. :D

gitmate-bot · 2017-03-07T17:14:56Z

tests/general/licensecheck_test_files/mit_license.py

+def add_two_numbers(a, b):
+    return a + b
+
+print(add_two_numbers(4, 5))


E305 expected 2 blank lines after class or function definition, found 1'

PycodestyleBear (E305), severity NORMAL, section autopep8.

yash-nisar · 2017-03-07T17:14:58Z

Done @Makman2 :)

Makman2 · 2017-03-07T17:47:09Z

bears/general/LicenseCheckBear.py

+    of the file for text belonging to various licenses.
+
+    For Ubuntu/Debian users, the `licensecheck_lines` option has to be used in
+    accordance with the `licensecheck_tail` option.


we use double backticks, and I'm not sure if RST handles single ones properly. Could you change that everywhere? :)

I'm sorry. :(

Makman2 · 2017-03-07T17:47:30Z

bears/general/LicenseCheckBear.py

+            file (and ignore `licensecheck_tail`).
+        :param licensecheck_tail:
+            Specify how many bytes to parse at end of file. Set to 0 to disable
+             parsing from end of file.


indentation off

How did I forget that. 😕

Makman2 · 2017-03-07T17:49:14Z

tests/general/LicenseCheckBearTest.py

+                                'No license found.',
+                                file=get_testfile_path('no_license.py'))],
+            filename=get_testfile_path('no_license.py'),
+            settings={'licensecheck_lines': 'int = 10'})


int = 10 ?

I think you mean just '10'

will check the first 10 lines and parse 5000 bytes from the bottom of the file. :)

but the int is invalid, isn't it? it's parsed as a string then^^

Yes @Makman2 , you are absolutely correct, actually I had copied that statement from the docs and added quotes because of some error in haste. Have filed an issue for the same. :D

Makman2 · 2017-03-08T16:01:12Z

bears/general/LicenseCheckBear.py

+
+@linter(executable='licensecheck',
+        output_format='regex',
+        output_regex=r'(^(.*/)*)([^/]*): .*UNKNOWN$',


for paths I would use .*, as this may not work on windows ;)

(we maybe need some regex composition library, where you could do something like regexes.path + r': .*UNKNOWN$')

(just an idea, don't seach for one ;D)

I had the same thought initially but we don't have support for windows so..

So now our bear is future proof. :D

Closes coala#823

yash-nisar · 2017-03-08T17:52:46Z

Done @Makman2 :D

Makman2 · 2017-03-08T19:52:21Z

ack 01d4f24

Makman2 · 2017-03-08T19:54:11Z

@rultor merge

rultor · 2017-03-08T19:55:36Z

@rultor merge

@Makman2 OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2017-03-08T19:57:21Z

@rultor merge

@Makman2 Done! FYI, the full log is here (took me 2min)

jayvdb · 2017-03-09T00:56:18Z

bears/general/LicenseCheckBear.py

+
+@linter(executable='licensecheck',
+        output_format='regex',
+        output_regex=r'.*: .*UNKNOWN$',


sigh.
The first .* should have been something like [^:]+.

gitmate-bot added process/pending review size/L labels Feb 18, 2017

yash-nisar force-pushed the test branch from 2ddce93 to 3c40ae5 Compare February 18, 2017 07:16

gitmate-bot added the review carefully! label Feb 18, 2017

yash-nisar force-pushed the test branch 6 times, most recently from 423b324 to 2c226ae Compare February 18, 2017 15:15

aptrishu added process/wip and removed process/pending review labels Feb 18, 2017

yash-nisar force-pushed the test branch from 2c226ae to e2b20e8 Compare February 18, 2017 18:52

gitmate-bot added process/pending review and removed process/wip labels Feb 18, 2017

yash-nisar force-pushed the test branch from e2b20e8 to 33c7257 Compare February 18, 2017 22:09

adtac reviewed Feb 19, 2017

View reviewed changes

yash-nisar force-pushed the test branch from 33c7257 to 0dfe3e9 Compare February 19, 2017 06:25

yash-nisar force-pushed the test branch from 0dfe3e9 to 3e082bd Compare February 19, 2017 06:34

sils requested changes Feb 20, 2017

View reviewed changes

sils added process/wip and removed process/pending review labels Feb 20, 2017

yash-nisar force-pushed the test branch from 3e082bd to 5f2db68 Compare February 23, 2017 20:23

gitmate-bot added process/pending review and removed process/wip labels Feb 23, 2017

gitmate-bot reviewed Feb 23, 2017

View reviewed changes

Makman2 reviewed Mar 6, 2017

View reviewed changes

yash-nisar force-pushed the test branch from 353fac8 to 0f8696d Compare March 7, 2017 17:13

gitmate-bot reviewed Mar 7, 2017

View reviewed changes

yash-nisar force-pushed the test branch from 0f8696d to b8a6638 Compare March 7, 2017 17:17

Makman2 reviewed Mar 7, 2017

View reviewed changes

yash-nisar force-pushed the test branch 2 times, most recently from a62fdf5 to 6f06fc4 Compare March 8, 2017 11:15

Makman2 reviewed Mar 8, 2017

View reviewed changes

bears/general: Add LicenseCheckBear

01d4f24

Closes coala#823

yash-nisar force-pushed the test branch from 6f06fc4 to 01d4f24 Compare March 8, 2017 17:24

Makman2 added process/approved and removed process/pending review labels Mar 8, 2017

rultor merged commit 01d4f24 into coala:master Mar 8, 2017

jayvdb reviewed Mar 9, 2017

View reviewed changes

yash-nisar deleted the test branch March 9, 2017 02:48

bears/general: Add LicenseCheckBear #1442

bears/general: Add LicenseCheckBear #1442

Conversation

yash-nisar commented Feb 18, 2017 • edited Loading

Checklist

yash-nisar commented Feb 18, 2017

yash-nisar commented Feb 18, 2017

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

adtac Feb 25, 2017 • edited Loading

Choose a reason for hiding this comment

yash-nisar Feb 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yash-nisar commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yash-nisar Feb 23, 2017 • edited Loading

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

yash-nisar commented Mar 6, 2017

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

Makman2 Mar 6, 2017 • edited Loading

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

yash-nisar commented Mar 7, 2017

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

Makman2 Mar 8, 2017 • edited Loading

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

yash-nisar commented Mar 8, 2017

Makman2 commented Mar 8, 2017

Makman2 commented Mar 8, 2017

rultor commented Mar 8, 2017

rultor commented Mar 8, 2017

jayvdb Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

yash-nisar commented Feb 18, 2017 •

edited

Loading

adtac Feb 25, 2017 •

edited

Loading

yash-nisar Feb 25, 2017 •

edited

Loading

yash-nisar Feb 23, 2017 •

edited

Loading

Makman2 Mar 6, 2017 •

edited

Loading

Makman2 Mar 8, 2017 •

edited

Loading

jayvdb Mar 9, 2017 •

edited

Loading