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

Remove false positive error of missing member in TextChoices tuples #298

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

moritz89
Copy link

@moritz89 moritz89 commented Dec 6, 2020

Why:

  • The fields label and value are not marked as missing

This change addresses the need by:

  • Adding a test to ensure that pylint does not mark these as errors

Why:

- Test that the fields label and value are not marked as missing

This change addresses the need by:

- Adding a test to ensure that pylint does not mark these as errors
@coveralls
Copy link

coveralls commented Dec 6, 2020

Pull Request Test Coverage Report for Build 1354

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-86.9%) to 0.0%

Totals Coverage Status
Change from base Build 1349: -86.9%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@atodorov
Copy link
Contributor

atodorov commented Dec 6, 2020

@moritz89 test looks good to me. Are you going to work on a fix next ?

@moritz89
Copy link
Author

moritz89 commented Dec 6, 2020

Hi, definitely. Could you give me a pointer to how this would be implemented or where I could gather examples of how this case can be handled?

@atodorov
Copy link
Contributor

atodorov commented Dec 6, 2020

Hi, definitely. Could you give me a pointer to how this would be implemented or where I could gather examples of how this case can be handled?

See comments on the issue, they give enough pointers. The only other thing I can point out to is http://pylint.pycqa.org/en/latest/how_tos/transform_plugins.html

@moritz89
Copy link
Author

moritz89 commented Jan 7, 2021

As part of the transform, I have to call manager.register_transform(nodes.Call, inference_tip(<transform_fcn>), <predicate fcn>) in an add_transforms(manager) function. However, I'm having trouble with the predicate function as it never triggers for the code in the test. Methods I've used to verify this include:

file_names = set()

def transform_fcn(node):
    file_name = node.root().file
    if file_name not in file_names:
        file_names.update({file_name})
        import ipdb; ipdb.set_trace()
    else:
        return False

and

class_names = set()

def is_tuple_in_text_choices(node):
    if not (isinstance(node.parent, nodes.Assign)
            or isinstance(node.parent, nodes.FunctionDef)) :
        return False
    if not isinstance(node.parent.parent, nodes.ClassDef):
        return False
    if "TextChoices" in [i.name for i in node.parent.parent.bases]:
        import ipdb; ipdb.set_trace()

Would appreciate a pointer for where I'm going wrong. The full file I'm using to apply the transform:

# pylint_django/transforms/text_choices.py

from astroid import MANAGER, scoped_nodes, nodes, inference_tip

from pylint_django import utils

class_names = set()

def is_tuple_in_text_choices(node):
    file_name = node.root().file
    if file_name not in file_names:
        file_names.update({file_name})
        import ipdb; ipdb.set_trace()
    else:
        return False

    if not (isinstance(node.parent, nodes.Assign)
            or isinstance(node.parent, nodes.FunctionDef)) :
        return False
    if not isinstance(node.parent.parent, nodes.ClassDef):
        return False
    if "TextChoices" in [i.name for i in node.parent.parent.bases]:
        import ipdb; ipdb.set_trace()

    class_name = node.parent.parent.name
    if class_name not in class_names:
        class_names.update({class_name})
        # import ipdb; ipdb.set_trace()
    else:
        return False
    
    # if node.parent.parent.name == "SomeTextChoices":
    #     import ipdb; ipdb.set_trace()
    # else:
    #     return False

    # if node.parent.parent.parent.name in ["TextChoices", "SomeClass", "ChoicesMeta", "TextChoices"]:
    #     import ipdb; ipdb.set_trace()
    # else:
    #     return False

    # if node.root().file.endswith("enums.py"):
    #     import ipdb; ipdb.set_trace()
    # else:
    #     return False

    # try:
    #     if node.root().file.endswith("model_enum.py"):
    #         import ipdb; ipdb.set_trace()
    # except:
    #     import ipdb; ipdb.set_trace()
    # if (node.parent.parent.name != "LazyObject"
    #         and node.parent.parent.parent.name != "django.db.models.expressions"
    #         and node.parent.parent.parent.name != "django.db.models.fields"):
    #     import ipdb; ipdb.set_trace()

    if isinstance(node.func, nodes.Attribute):
        attr = node.func.attrname
    elif isinstance(node.func, nodes.Name):
        attr = node.func.name
    else:
        return False
    return True


def infer_key_classes(node, context=None):
    pass


def add_transforms(manager):
    manager.register_transform(nodes.Call, inference_tip(infer_key_classes),
                               is_tuple_in_text_choices)

@atodorov
Copy link
Contributor

atodorov commented Jan 7, 2021

Would appreciate a pointer for where I'm going wrong. The full file I'm using to apply the transform:

Please submit what you think is your best implementation to this PR so I can debug without having to copy & paste.

One possible path is to try and suppress the no-member error message (depends on how exactly this pylint checker is written and where exactly this triggers for our examples). The path with ast transformations is probably more elegant although more rocky as well. In my experience they are not very well documented (or maybe just rarely used and not deeply understood, IDK). I need some quiet time to be able to dig into your implementation and figure out what's happening.

Why:

- Transform tuples to include value and label properties

This change addresses the need by:

- Adding the text_choices transform file
- Integrating in the transforms init file
- Focusing on the predicate for the rule to trigger the transform
@moritz89
Copy link
Author

Hi Alexander, I pushed the code with which I try to trigger the transform in the test class. The trouble I have is that during the parsing process I never seem to land in the test file itself

@atodorov
Copy link
Contributor

Hi Alexander, I pushed the code with which I try to trigger the transform in the test class. The trouble I have is that during the parsing process I never seem to land in the test file itself

From what I can tell this is because you are triggerring the AST transform on a Call node, which the references to .value isn't. Hence nothing of what you wrote in the transform module makes any difference.

Something.other_thing.value is an Attribute node. The condition/pattern here is an Attribute (you may or may not want to limit attrname to value/label) whose .expr is another Attribute whose .expr is Name, which when .inferred() points to a ClassDef. Name.name & ClassDef.name have the same value. This is the model class definition. In this example SomeTextChoices.

Now SomeTextChoices inherits from models.TextChoices which itself inherits from enum.Choices which wraps up the built-in type enum.Enum.

In Django you have this signature class Choices(enum.Enum, metaclass=ChoicesMeta): and the ChoicesMeta reveals the cls.choices attribute. Then in __new__ we have cls.label = property(lambda self: cls._value2label_map_.get(self.value)).

This is how what you declare as a tuple gets a .label property. Not yet sure where .values comes from.

I'm not quite sure what the rest of your code does and why. You check for assign nodes, classdef, funcdef but I fail to see the idea behind. The code being not complete doesn't help either.

I did experiment with the following snippet:

def inherits_from_choices_class(node):
    return node_is_subclass(node,
        'django.db.models.Choices',
        'django.db.models.enums.Choices',
        '.Choices',
    )


def transform_choice_tuples(node):
    print("**** node=", node, node.locals)

    for local in node.locals:
        if local.startswith('__'):
            continue

        print("--- local=", local)

#    for member in node.member:
#        member.value = ""
#        member.label = ""
    return node


def add_transforms(manager):
    manager.register_transform(nodes.ClassDef, transform_choice_tuples,
                               inherits_from_choices_class)

It will trigger the transform function when the class in question inherits from Choices. Then you can loop around all members (e.g. all tuple attributes) and just set a value/label attributes on them and pylint will stop complaining.

I have some examples in this old talk - https://www.youtube.com/watch?v=3CkSKUNMLJc&list=PLFjlI7p-h1hxBP3cIjEqePSeoBDHud5Db&index=47&t=328s (after 40:00). IIRC I took them directly from pylint's own source code.

Hope this helps.

@carlio carlio force-pushed the master branch 5 times, most recently from aa6c7ee to 054b49a Compare May 15, 2023 03:57
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

Successfully merging this pull request may close these issues.

3 participants