[IMP] pylint-odoo: Adding support for parameters --ignore and --ignore-patterns #103

JesusZapata · 2017-01-05T18:04:15Z

pylint supports the following parameters:

--ignore=<file>[,<file>...]
--ignore-patterns=<pattern>[,<pattern>...]

But pylint-odoo is not using for ignore them from custom search files methods.
We are searching files: js, md, rst...
And we need skip them too if this parameters is defined.

Doubts you have:

How do I run the tests with the console and append more parameters --ignore and / or --ignore-patterns.
When running with "python setup.py test" the setup.py file can not pass more arguments.
Running it with "tox" to the tox command can not pass more arguments.

Related to issue #102

JesusZapata · 2017-01-05T21:18:30Z

pylint_odoo/misc.py

        """
        self.ext_files = {}
+        base_path = os.path.dirname(os.path.realpath(__file__))
+        ignores = []
+        ignore_argvs = getopt.getopt([argv for argv in sys.argv[1:]


Parameters are received with sys.argv but when run with setup.py or tox that variable is empty.
Could you help me with this @moylop260 ?

The parameters are received from config like as other custom parameters.
Because a configfile could be used too.

Search where is used the config variable to read by example the js configuration file for js-lint check

JesusZapata · 2017-01-06T14:58:19Z

pylint_odoo/checkers/modules_odoo.py

@@ -237,6 +237,20 @@ class ModuleChecker(misc.WrapperModuleChecker):
                     'javascript lint. You can use the environment variable '
                     '"PYLINT_ODOO_JSLINTRC" too. Default: %s' % DFTL_JSLINTRC)
        }),
+        ('po-ignore', {


I try to use --ignore but enter in conflict with ignore of pylint
It is a good solucion?

moylop260 · 2017-01-06T15:10:51Z

Yeah Its right. The idea is use the same parameter of pylint. El 06/01/2017 8:58 a.m., "Jesus Zapata" <notifications@github.com> escribió:

…

***@***.**** commented on this pull request. ------------------------------ In pylint_odoo/checkers/modules_odoo.py <#103 (review)> : > @@ -237,6 +237,20 @@ class ModuleChecker(misc.WrapperModuleChecker): 'javascript lint. You can use the environment variable ' '"PYLINT_ODOO_JSLINTRC" too. Default: %s' % DFTL_JSLINTRC) }), + ('po-ignore', { I try to use --ignore but enter in conflict with ignore of pylint It is a good solucion? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#103 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGVh27phRqZHWzmd3bxVt1K0h83sqhpNks5rPlaLgaJpZM4Lb9ja> .

JesusZapata · 2017-01-06T20:08:01Z

Now used the correct parameters --ignore and --ignore-patterns !

moylop260 · 2017-01-07T17:12:19Z

pylint_odoo/misc.py

+        ignores_patterns = [glob.glob(os.path.join(base_path, ignore.pattern))
+                            for ignore
+                            in self.linter.option_value('ignore-patterns')]
+        for ignore in ignores_patterns:


What about use utils._basename_in_blacklist_re method?

Okay! Thanks for the information of this method ! I started using it!

moylop260 · 2017-01-07T17:14:41Z

pylint_odoo/misc.py

+                   for ignore in self.linter.option_value('ignore')]
+        ignores_patterns = [glob.glob(os.path.join(base_path, ignore.pattern))
+                            for ignore
+                            in self.linter.option_value('ignore-patterns')]


I'm not tested this code but I don't understand how works if the arg.dest for this parameter is black_list_re
Check if It have the same content, please

Very well, I did not know that parameter! I'm using it now

moylop260 · 2017-01-07T17:19:24Z

pylint_odoo/misc.py

        """
        self.ext_files = {}
+        base_path = os.path.dirname(os.path.realpath(__file__))


Why we need the path of pylint_odoo/misc.py file here?

I used another variable what store same value!

I try to use another variable and do not pass the tests

Add a commenr in the code of why is required this value

Note: "because do not pass the test" is not a good justify

if you run manually the command pylint with ignore with other path won't work.
I mean, copy test_repo to /tmp/ and check if ignore parameters are working yet.

moylop260 · 2017-01-07T17:23:00Z

pylint_odoo/misc.py

+                if ignore in root:
+                    find = False
+                    continue
+            if find:


The cyclomatic complexity of this method is too high.
Could you refactoring it to reduce it?

I'm not sure why this array should be traversed!

But with the continue breaks the cycle

…e-patterns#

moylop260 · 2017-01-09T00:17:32Z

pylint_odoo/misc.py

+                                                      self.linter.config.
+                                                      black_list_re):
+                        find = False
+                        continue


A continue as last sentence of a for is useless.
Maybe you want use break

moylop260 · 2017-01-09T00:20:54Z

pylint_odoo/test/main.py

+                        '--enable=javascript-lint']
+        pylint_res = self.run_pylint(self.paths_modules, extra_params)
+        real_errors = pylint_res.linter.stats['by_msg']
+        self.assertEqual(real_errors.items(), [])


You should force a output in order to validate that was skipped files.

I changed test cases!

JesusZapata · 2017-01-09T04:18:49Z

I made test into the folder /tmp attached screenshot

Firts all deprecated-openerp-xml-node

Second parameter --ignore

Third parameter --ignore-patterns

Is good idea used deprecated-openerp-xml-node for test this functionality ?

moylop260 · 2017-01-09T17:53:02Z

Maybe is working the original parameter, because you are skipping a full folder.
You should try with a particular file.

And remove the following line: base_path = os.path.dirname(os.path.realpath(__file__))

…ables

JesusZapata · 2017-01-09T21:03:33Z

@moylop260 Thanks for you observation! The previous code did not find the files

moylop260 · 2017-01-10T09:03:19Z

pylint_odoo/misc.py

                fext = os.path.splitext(filename)[1].lower()
                fname_rel = os.path.relpath(
                    os.path.join(root, filename), self.module_path)
-                self.ext_files.setdefault(fext, []).append(fname_rel)
+                name = os.path.join(self.module_path, fname_rel)


Are you deleting a value to add it again?
I mean, you have os.path.join(root, filename):

e.g. /mypath1/mypath2/myfile.ext

Then you get relative path based on self.module_path

e.g. If self.module_path is /mypath1 then the relative path is mypath2/myfile.ext after that you are joining again the self.module_path to get /mypath1/mypath2/myfile.ext again.

Do you want drive me crazy or Am I missing something?

I don't understand.

When run pylint -r n --disable=all --enable=deprecated-openerp-xml-node --ignore=test_module/res_users.xml --load-plugins=pylint_odoo /tmp/test_repo/

The variable self.linter.config.black_list has ['test_module/res_users.xml']
I only ask if ignore in name: the name variable has '/tmp/test_repo/test_module/res_users.xml'

Does this help you?

Add a pdb and print the value os.path.join(root, filename) and name and tell me what is the difference

Output of pdb

(Pdb++) os.path.join(root, filename) '/tmp/test_repo/test_module/res_users.xml' (Pdb++) name '/tmp/test_repo/test_module/res_users.xml'

They are the same values!
I can remove the variable name and use os.path.join (root, filename) directly

Is that what you mean?

I told you
You are using a=10; a+=1; a-=1 😞

Ok, thanks to you, i have fixed it!

moylop260 · 2017-01-10T09:08:55Z

pylint_odoo/misc.py

+                # If the file is within ignores is ignored
+                for ignore in self.linter.config.black_list:
+                    if ignore in name:
+                        find = False


What means find variable?
(Because I don't understand why we have a find = False when is really find it)

I have fixed this, I give a better use to the variable

JesusZapata · 2017-01-11T15:28:06Z

@moylop260
What else does it take to close this PR?

moylop260 · 2017-01-11T17:20:57Z

pylint_odoo/misc.py

                fext = os.path.splitext(filename)[1].lower()
                fname_rel = os.path.relpath(
                    os.path.join(root, filename), self.module_path)
-                self.ext_files.setdefault(fext, []).append(fname_rel)
+                # If the file is within black_list_re is ignored


fname = os.path.join(root, filename)
And reuse this variable 2 times

I have used the variable when needed!

moylop260 · 2017-01-11T18:26:12Z

pylint_odoo/misc.py

+                        find = True
+                        break
+                if not find:
+                    self.ext_files.setdefault(fext, []).append(fname_rel)


Assign fname_rel variable here where is really used.

moylop260 · 2017-01-11T18:28:28Z

pylint_odoo/misc.py

        """
        self.ext_files = {}
+        ignores = list(self.linter.config.black_list)


Why we need this variable instead of use it directly?
NOTE: I requested a explanation, don't confuse with a request of change.

The variable is not necessary since it is used only once!
This change had been made by the 188d405 commit excluding the test folder
Can I return this change?

Thanks for clarify.
Yes, please

moylop260

LGTM just waiting justification of variable ignores

moylop260 · 2017-01-11T19:07:01Z

Please create PR to oca from merged sha

…e-patterns (Vauxoo#103)

JesusZapata · 2017-01-11T19:33:55Z

Done the PR OCA#103

…-for-ignore-files-jesuszapata [IMP] pylint-odoo: Adding support for parameters --ignore and --ignore-patterns

JesusZapata commented Jan 5, 2017

View reviewed changes

JesusZapata force-pushed the master-add-supports-for-ignore-files-jesuszapata branch from 84f871a to 25cc9d0 Compare January 6, 2017 14:52

JesusZapata commented Jan 6, 2017

View reviewed changes

JesusZapata force-pushed the master-add-supports-for-ignore-files-jesuszapata branch from 86df315 to a442815 Compare January 6, 2017 20:58

moylop260 reviewed Jan 7, 2017

View reviewed changes

JesusZapata force-pushed the master-add-supports-for-ignore-files-jesuszapata branch 2 times, most recently from 1ddde14 to 1c88591 Compare January 8, 2017 22:29

[IMP] pylint-odoo: Adding support for parameters --ignore and --ignor…

5a5375f

…e-patterns#

JesusZapata force-pushed the master-add-supports-for-ignore-files-jesuszapata branch from 910e124 to 5a5375f Compare January 8, 2017 23:02

moylop260 reviewed Jan 9, 2017

View reviewed changes

[FIX] pylint-odoo: Optimization and changes on the test case

99999d7

[FIX] pylint-odoo: Fix pep8

532a3f2

[FIX] pylint-odoo: Fix --ignore parameter and delete unnecessary vari…

7ee4551

…ables

moylop260 reviewed Jan 10, 2017

View reviewed changes

JesusZapata added 3 commits January 10, 2017 12:43

[FIX] pylint-odoo : Fix best use of find variable

6fd9781

[FIX] pylint-odoo: Delete variables that have the same content!

d8d8be4

[IMP] pylint-odoo: Add in list to exclude the test folder

188d405

[REF] misc.py: Skip test will be created from other PR

e339922

moylop260 reviewed Jan 11, 2017

View reviewed changes

[FIX] pylint-odoo: Use variables when possible

4e70227

moylop260 reviewed Jan 11, 2017

View reviewed changes

Update misc.py

9821df0

moylop260 reviewed Jan 11, 2017

View reviewed changes

moylop260 approved these changes Jan 11, 2017

View reviewed changes

JesusZapata added 2 commits January 11, 2017 18:38

[FIX] pylint-odoo: Delete unnecessary variable

dde3d6e

[FIX] pylint-odoo: Solving conflicts

ec722f6

JesusZapata force-pushed the master-add-supports-for-ignore-files-jesuszapata branch from 97fbd70 to ec722f6 Compare January 11, 2017 18:51

moylop260 merged commit 9762664 into Vauxoo:master Jan 11, 2017

moylop260 deleted the master-add-supports-for-ignore-files-jesuszapata branch January 11, 2017 19:06

JesusZapata pushed a commit to vauxoo-dev/pylint-odoo that referenced this pull request Jan 11, 2017

[IMP] pylint-odoo: Adding support for parameters --ignore and --ignor…

82feead

…e-patterns (Vauxoo#103)

moylop260 mentioned this pull request Feb 10, 2017

[FIX] Active xml-attribute-translatable if the version is 8.0 #111

Merged

[IMP] pylint-odoo: Adding support for parameters --ignore and --ignore-patterns #103

[IMP] pylint-odoo: Adding support for parameters --ignore and --ignore-patterns #103

Conversation

JesusZapata commented Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moylop260 commented Jan 6, 2017 via email

JesusZapata commented Jan 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

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

JesusZapata commented Jan 9, 2017

moylop260 commented Jan 9, 2017

JesusZapata commented Jan 9, 2017

moylop260 Jan 10, 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

Choose a reason for hiding this comment

moylop260 Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesusZapata commented Jan 11, 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

moylop260 left a comment

Choose a reason for hiding this comment

moylop260 commented Jan 11, 2017

JesusZapata commented Jan 11, 2017 • edited Loading

JesusZapata commented Jan 5, 2017 •

edited

Loading

moylop260 Jan 10, 2017 •

edited

Loading

moylop260 Jan 10, 2017 •

edited

Loading

JesusZapata commented Jan 11, 2017 •

edited

Loading