Internal updates caused by pylint/astroid 2.0. Fixes #182. #183

atodorov · 2018-07-23T11:12:33Z

Notes:

The first commit will clash with the latest commit on top of master. I rewrote the commit for a shorter implementation and then realized we had already merged the earlier implementation.
Take a look at the new way we apply transformations. I'm not quite certain this is what we need to be doing here but it fixed couple of failures, namely:
https://travis-ci.org/PyCQA/pylint-django/builds/405211436
func_noerror_model_fields and func_noerror_ugettext_lazy_format

atodorov · 2018-07-23T11:28:41Z

More notes:

For the failing test func_noerror_foreignkeys I see:

E           Failed: Wrong results for file "func_noerror_foreignkeys":
E           
E           Unexpected in testdata:
E             27: no-member

If I execute pylint-django only against this file everything seems fine:

$ PYTHONPATH=. pylint --load-plugins=pylint_django pylint_django/tests/input/func_noerror_foreignkeys.py 

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

I don't know what is going on here.

Update: this seems to work on Python 3.4 !!!!

atodorov · 2018-07-23T21:17:12Z

Excluding the conflict I now have only 1 failing test. If I apply the following diff:

--- a/pylint_django/tests/test_func.py
+++ b/pylint_django/tests/test_func.py
@@ -43,6 +43,8 @@ def get_tests(input_dir='input', sort=False):
 
     suite = []
     for fname in os.listdir(input_dir):
+        if fname.find('func_noerror_foreign') == -1:
+            continue
         if fname != '__init__.py' and fname.endswith('.py'):
             suite.append(test_functional.FunctionalTestFile(input_dir, fname))

then I get only a subset of the tests executed (including the failing one) and this time it passes. This looks very strange.

PCManticore

This change looks sane to me but I don't have enough context on pylint-django to understand more. If you have a particular use case we should look into, let me know!

atodorov · 2018-07-24T09:41:33Z

@PCManticore any idea why I get varying results depending on how I execute one of the tests ?

carlio · 2018-07-24T09:42:38Z

pylint_django/transforms/__init__.py

+
+# TODO: no sure what to do with commented out code! It looks like
+# we don't need these transforms anymore
+


I'd suggest either leave them in or just a comment with a commit hash and an explanation of 'we removed some transforms' in case new issues get opened wanting them back.

carlio · 2018-07-24T09:43:21Z

Code seems to make sense to me but we're still getting build failures.

for checking if a module is named wsgi.py or includes 'migrations' in its name we take the first element of the list

According to this documentation http://pylint.pycqa.org/projects/astroid/en/latest/extending.html?highlight=MANAGER#module-extender-transforms the module transform function doesn't accept any parameters and returns a new Module node.

after the previous commit these transformation don't seem to be needed. Maybe the latest pylint/astroid is better at figuring out the internals of these Django classes. Additionally things like django_contrib_postgres_fields.py don't seem to be hooked up anywhere so remove them as well.

previous augmentation was not handling factory objects very well. This is an ammendment to that. I'm not exactly certain why we haven't seen other failures before.

coveralls · 2018-07-24T21:16:52Z

Pull Request Test Coverage Report for Build 451

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 87.023%

Totals
Change from base Build 445:	0.0%
Covered Lines:	570
Relevant Lines:	655

💛 - Coveralls

atodorov · 2018-07-24T21:24:49Z

@carlio I vote for merging this change if you don't mind. ATM I am at a total loss how to debug the failing test (IMO this is a flaky test) and I've posted the symptoms to code-quality ML hoping for some hints.

The failing `func_noerror_foreignkeys` was caused by pylint_django trying to infer a ForeignKey('Author') field by looking at the astroid cache. In this case there was an Author model already defined in `func_noerror_duplicate_except_doesnotexist` which was inferred and of course this model didn't have the `author_name` field hence we got a no-member error. This commit tries to restrict where we load these models from and also takes into account the quirk that Django allows specifying 'appname.Model' instead of 'path.to.python.module.models.Model'.

atodorov · 2018-07-24T22:44:33Z

@carlio sorry for the noise. Finally figured out what was wrong with the test. See the last commit.

I am up for a release tomorrow, any preferences for teh version number ? Should we also do 2.0 or keep it incremental (0.12 then).

carlio · 2018-07-25T04:04:05Z

@atodorov Yes I think it's time to have a better version scheme. It just started as I used '0' to mean 'not quite sure this is done yet' and never changed it. I suppose it might as well follow pylint more closely but at least it could get a major version number now I think.

carlio · 2018-07-25T04:05:33Z

@atodorov and if that number changes, could probably remove the beta classifier at the same time.

atodorov requested a review from carlio July 23, 2018 11:12

atodorov mentioned this pull request Jul 23, 2018

HIGH: Failing tests with pylint 2.0 and astroid 2.0 #182

Closed

atodorov requested a review from PCManticore July 23, 2018 21:17

PCManticore approved these changes Jul 24, 2018

View reviewed changes

carlio reviewed Jul 24, 2018

View reviewed changes

atodorov added 5 commits July 24, 2018 23:55

Module.__path__ is now a list

bb1bd18

for checking if a module is named wsgi.py or includes 'migrations' in its name we take the first element of the list

New implementation of module transforms

0542236

According to this documentation http://pylint.pycqa.org/projects/astroid/en/latest/extending.html?highlight=MANAGER#module-extender-transforms the module transform function doesn't accept any parameters and returns a new Module node.

Handle attributes of type DjangoModelFactory

f3d2890

previous augmentation was not handling factory objects very well. This is an ammendment to that. I'm not exactly certain why we haven't seen other failures before.

Unify is_model_factory_meta_subclass() and is_model_meta_subclass

3fdcbcc

atodorov force-pushed the pylint20 branch from a506060 to 3fdcbcc Compare July 24, 2018 21:06

carlio approved these changes Jul 25, 2018

View reviewed changes

atodorov merged commit 7a77887 into master Jul 25, 2018

atodorov deleted the pylint20 branch July 25, 2018 06:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Internal updates caused by pylint/astroid 2.0. Fixes #182. #183

Internal updates caused by pylint/astroid 2.0. Fixes #182. #183

Uh oh!

atodorov commented Jul 23, 2018

Uh oh!

atodorov commented Jul 23, 2018 •

edited

Loading

Uh oh!

atodorov commented Jul 23, 2018

Uh oh!

PCManticore left a comment

Uh oh!

atodorov commented Jul 24, 2018

Uh oh!

carlio Jul 24, 2018

Uh oh!

carlio commented Jul 24, 2018

Uh oh!

coveralls commented Jul 24, 2018 •

edited

Loading

Uh oh!

atodorov commented Jul 24, 2018

Uh oh!

atodorov commented Jul 24, 2018

Uh oh!

carlio commented Jul 25, 2018

Uh oh!

carlio commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants


		# TODO: no sure what to do with commented out code! It looks like
		# we don't need these transforms anymore

Uh oh!

Internal updates caused by pylint/astroid 2.0. Fixes #182. #183

Internal updates caused by pylint/astroid 2.0. Fixes #182. #183

Uh oh!

Conversation

atodorov commented Jul 23, 2018

Uh oh!

atodorov commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atodorov commented Jul 23, 2018

Uh oh!

PCManticore left a comment

Choose a reason for hiding this comment

Uh oh!

atodorov commented Jul 24, 2018

Uh oh!

carlio Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

carlio commented Jul 24, 2018

Uh oh!

coveralls commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 451

💛 - Coveralls

Uh oh!

atodorov commented Jul 24, 2018

Uh oh!

atodorov commented Jul 24, 2018

Uh oh!

carlio commented Jul 25, 2018

Uh oh!

carlio commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

atodorov commented Jul 23, 2018 •

edited

Loading

coveralls commented Jul 24, 2018 •

edited

Loading