Fix license check #28

lmr · 2017-08-09T14:07:06Z

Fixes #27, reported by @rdkdd.

Signed-off-by: Lucas Meneghel Rodrigues <lucas.rodrigues@profitbricks.com>

The current code has a bug reporting license check when --fix is not passed. Let's fix it and also properly report when the license check has failed. Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

Add a new option --no-license-check to checkall, so that people working on non gplv2 projects can simply skip the check. Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

ldoktor

Yep, looks and works good.

ldoktor · 2017-08-09T14:27:32Z

inspektor/utils/process.py

-    result.stdout = stdout
-    result.stderr = stderr
+    result.stdout = str(stdout)
+    result.stderr = str(stderr)


Why is this required? Isn't the return of communicate already string?

In py3 it's bytes :)

ldoktor · 2017-08-09T14:29:16Z

inspektor/lint.py

@@ -63,8 +69,9 @@ def get_opts(self):
                pylint_args.append('--disable=%s' % self.ignored_errors)
            if self.enabled_errors:
                pylint_args.append('--enable=%s' % self.enabled_errors)
-            pylint_args.append('--reports=no')
-            if sys.version_info[:2] > (2, 6):
+            if self._pylint_has_option('--reports'):


Yep, this should be safest. I'd prefer if we check --reports=<y to avoid possible clashes with --reports-whatever or even just example --reports=n which might be listed in some comment, but that is probably just paranoiac...

Fair enough

ldoktor · 2017-08-09T14:30:15Z

inspektor/lint.py

@@ -45,10 +46,15 @@ def __init__(self, args, logger=logging.getLogger('')):
            self.log.info('Pylint enabled : %s' % self.enabled_errors)
        else:
            self.log.info('Verbose mode, no disable/enable, full reports')
+        help_result = process.run('pylint --help')


Well this looks like a global thing and not per-object one. How about moving this to the top and evaluate only once per import?

Added a954a8a that implements the suggestions you made.

ldoktor · 2017-08-09T15:11:21Z

inspektor/lint.py

-            if sys.version_info[:2] > (2, 6):
+            if self._pylint_has_option('--reports='):
+                pylint_args.append('--reports=no')
+            if self._pylint_has_option('--score='):


OK, hopefully they won't change that format 😄

ldoktor · 2017-08-09T15:13:08Z

inspektor/lint.py

@@ -19,6 +19,9 @@
 from pylint.lint import Run

 from .path import PathChecker
+from .utils import process
+
+_PYLINT_HELP_TEXT = process.run('pylint --help').stdout


I just noticed this outputs unnecessary Running... message. Could you please silence it? Apart from that it looks and work good (as far as my old pylint without --score says)

Let's use a heuristic that tends to be more reliable instead of looking for python versions or pylint version strings: Check directly the --help output. Since in most py apps the output is auto generated from real command line options, this should be relatively safe. This solves a problem with moving target options such as --score and others. Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

We don't need to print the innocuous bits of information about the commands and applications being initialized. Let's get rid of those methods. Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

On regular mode, only log FAILed files, but on verbose mode, log PASSed as well. Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

For fixable content, whenever we fix a file, let's report on wether the fix was OK or not. This will avoid confusion of people running inspektor and it silently fixing files, and next run the same check will pass (because it was fixed in the previous execution). Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

ldoktor · 2017-08-10T06:23:22Z

inspektor/cli/app.py

    def clean_up(self, cmd, result, err):
-        self.LOG.debug('clean_up %s', cmd.__class__.__name__)


yep, at this point they are useless...

ldoktor · 2017-08-10T06:25:39Z

inspektor/path.py

+        if status == 'PASS':
+            self.log.debug('%s: %s %s', self.label, self.path, status)
+        elif status == 'FAIL':
+            self.log.error('%s: %s %s', self.label, self.path, status)


Else: custom status or ValueError might unveil typos...

ldoktor · 2017-08-10T07:19:22Z

inspektor/style.py

            if AUTOPEP8_CAPABLE:
                if self.args.fix:
                    self.log.info('Trying to fix errors with autopep8')
                    try:
-                        process.run('autopep8 --in-place --max-line-length=%s --ignore %s %s' % (self.args.max_line_length, self.args.disable, path))
+                        process.run('autopep8 --in-place --max-line-length=%s --ignore %s %s' % (self.args.max_line_length, self.ignored_errors, path))


This looks like a fix not related to this commit...

Yep, I'm a lazy bastard :) Sorry for that one.

lmr and others added 3 commits August 9, 2017 14:48

inspektor.utils.data_structures: Fix lack of license header

10037ac

Signed-off-by: Lucas Meneghel Rodrigues <lucas.rodrigues@profitbricks.com>

inspektor.license: Fail license check when --fix is not passed

9d66cc5

The current code has a bug reporting license check when --fix is not passed. Let's fix it and also properly report when the license check has failed. Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

inspektor.commands.checkall: Introduce --no-license-check option

121f501

Add a new option --no-license-check to checkall, so that people working on non gplv2 projects can simply skip the check. Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

ldoktor approved these changes Aug 9, 2017

View reviewed changes

ldoktor reviewed Aug 9, 2017

View reviewed changes

lmr force-pushed the fix-license-check branch from 9847190 to a954a8a Compare August 9, 2017 14:38

ldoktor reviewed Aug 9, 2017

View reviewed changes

lmr added 4 commits August 9, 2017 22:01

inspektor.cli.app: Simplify base InspektorApp

a720ce7

We don't need to print the innocuous bits of information about the commands and applications being initialized. Let's get rid of those methods. Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

Standardize results reporting

4090f51

On regular mode, only log FAILed files, but on verbose mode, log PASSed as well. Signed-off-by: Lucas Meneghel Rodrigues <lookkas@gmail.com>

lmr force-pushed the fix-license-check branch from a954a8a to ae9d774 Compare August 9, 2017 21:45

lmr merged commit 3d5a14a into master Aug 9, 2017

ldoktor reviewed Aug 10, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix license check #28

Fix license check #28

lmr commented Aug 9, 2017

ldoktor left a comment

ldoktor Aug 9, 2017

lmr Aug 9, 2017

ldoktor Aug 9, 2017

ldoktor Aug 9, 2017

lmr Aug 9, 2017

ldoktor Aug 9, 2017

lmr Aug 9, 2017

lmr Aug 9, 2017

ldoktor Aug 9, 2017

ldoktor Aug 9, 2017

lmr Aug 9, 2017

ldoktor Aug 10, 2017

ldoktor Aug 10, 2017

ldoktor Aug 10, 2017

lmr Aug 10, 2017

		def clean_up(self, cmd, result, err):
		self.LOG.debug('clean_up %s', cmd.__class__.__name__)

Fix license check #28

Fix license check #28

Conversation

lmr commented Aug 9, 2017

ldoktor 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