Skip to content
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.

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

[pylint] E1101:Instance of 'ForeignKey' has no 'operator' member #142

Closed
debuggerpk opened this issue Mar 22, 2018 · 12 comments
Closed

[pylint] E1101:Instance of 'ForeignKey' has no 'operator' member #142

debuggerpk opened this issue Mar 22, 2018 · 12 comments

Comments

@debuggerpk
Copy link

debuggerpk commented Mar 22, 2018

as instructed in #21 , creating a new issue.

if i call self.discount.name twice on a model, it raises a linting error. See screenshot. the error raised is

[pylint] E1101:Instance of 'ForeignKey' has no 'operator' member

screen shot 2018-03-22 at 9 41 32 pm

WARNING: Please do not report issues about missing Django, see
README!

@debuggerpk debuggerpk changed the title does not handle django's ForeignKey well. [pylint] E1101:Instance of 'ForeignKey' has no 'operator' member Mar 22, 2018
@atodorov
Copy link
Contributor

Post the definition of the Discount model as plain text, not a screenshot.

@debuggerpk
Copy link
Author

class Discount(TimeStamped):
  name = models.CharField(max_length=255)
  value = models.DecimalField(max_digits=8, decimal_places=2, blank=True, null=True)
  operator = models.CharField(max_length=255, blank=True, null=True)
  is_active = models.NullBooleanField()

  customer = models.ForeignKey('customers.Customer', on_delete=models.PROTECT, related_name='discounts')

@atodorov
Copy link
Contributor

I tried with the following example (a bit different than yours)

from django.db import models


class Discount(models.Model):
    name = models.CharField(max_length=255)
    value = models.DecimalField(max_digits=8, decimal_places=2, blank=True, null=True)
    operator = models.CharField(max_length=255, blank=True, null=True)
    is_active = models.NullBooleanField()

class OutletDiscount(models.Model):
    discount = models.ForeignKey(Discount, on_delete=models.PROTECT)

    @property
    def name(self):
        return self.discount.name

    @property
    def operator(self):
        return self.discount.operator

    @property
    def value(self):
        return self.discount.value

I was not able to reproduce so closing.

Make sure you are using the latest version from PyPI and if you can still reproduce prepare a self contained example.

To do this place your code under the pylint_django/tests/input directory and execute scripts/test.sh. If you can manage to create a test file that reproduces this problem but does not complain about missing class definitions, etc then I can investigate more.

@debuggerpk
Copy link
Author

@atodorov the problem appears when the FK field references a string not the actual class. In my example, the class is

class OutletDiscount(models.Model):
  outlet = models.ForeignKey(Outlet, on_delete=models.PROTECT, related_name='discounts')
  discount = models.ForeignKey('warehouse.Discount', on_delete=models.PROTECT)

  @property
  def name(self):
    return self.discount.name

  @property
  def operator(self):
    return self.discount.operator

  @property
  def value(self):
    return self.discount.value

notice the discount field on OutletDiscount, it class the FK from string.

@debuggerpk
Copy link
Author

PS: i am sorry if it has been difficult for me to explain.

@dmytrokyrychuk
Copy link

I can confirm the issue reported by @ysfjwd.

@atodorov you were not able to reproduce because the referenced model was in scope. It appears that referenced models must be in astroid's cache for the string approach to work correctly, which seems to defeat the purpose of using strings completely.

@atodorov
Copy link
Contributor

@KyrychukD thanks for the tip. So the FK model both needs to be referenced as string and needs to live in an external file I think. With that I was able to reproduce but I have no idea how to fix that yet.

@atodorov atodorov reopened this May 24, 2018
@atodorov
Copy link
Contributor

So the current implementation is based on #35 and #119 adds few tests. These are working well in the case where the referenced model is defined in the same module so I guess it is in the astroid cache.

The current problem appears to be that when the module defining the model is external it isn't loaded and not available to astroid so the original implementation rightly fails because it has no idea that there is a class named 'module.Model'.

One thing we can do is try to import the referenced module before performing the check. @carlio how do you feel about this ? I'll try a POC of this idea to see if it works.

@atodorov
Copy link
Contributor

A few more thoughts on this:

  1. There's the obvious problem of discovering the module before it is imported which depends on the exact filesystem layout and PYTHONPATH/sys.path modifications you are doing in your project which pylint doesn't know about.

  2. assuming Python is able to find a module with the desired name I get a failure from Django:

E django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.

because I'm trying to load one piece of the application without loading the entire app first.

I'm starting to gravitate towards wontfix b/c there are many edge cases where this will not work and we'll have to either special-case them one by one or explain to people why it's not working and why it's not going to be fixed. IMO I'd rather go with a "don't reference models as strings" known issue approach and forget about this!

@dmytrokyrychuk
Copy link

dmytrokyrychuk commented May 24, 2018 via email

@atodorov
Copy link
Contributor

Is there a way in pylint to defer inspection somehow, e.g. until there's no more modules to load?

I don't think so. What if I execute pylint path/to/some/models.py and the foreign key models are defined into another file? That other file will never be loaded if it isn't imported anywhere.

And as you say the reason for using string names is circular dependencies between modules which is a bad sign for the project. IMO the best course of action will be to clean up the offending application and have a simpler architecture of the underlying models so that there are no circular dependencies.

@carlio any thoughts on this issue ?

@carlio
Copy link
Collaborator

carlio commented May 25, 2018

My thoughts on this remain that pylint (and therefore pylint-django) can only inspect code that exists and since there is no way to know what model strings are referring to without setting up and inspecting Django code, this seems like just a very tricky thing to do without overcomplicating this library.

I supposed I'd ask why pylint-django should be able to find the model if the models.py itself is not importing it but using a string to reference. Presumably there's some import problem preventing referencing the class directly, in which case pylint-django can't do a huge amount, or it's just done though convenience in which case that's really on the developer to fix.

Just my 2 cents :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants