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.

Sign up for GitHub

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

Jump to bottom

Make the ForeiginKey detection more accurate #330

Closed
wants to merge 1 commit into from
Closed

Make the ForeiginKey detection more accurate #330

wants to merge 1 commit into from

Conversation

qq915522927
Copy link

@qq915522927 qq915522927 commented Aug 2, 2021 •
edited
Loading

we have a api.py which contains a tastypie resource, pylint regonized the resource's ForeiginKey as django's ForeiginKey by mistake, and then cause an error.

class SavedSearchesResource(ModelResource):
    owner = fields.ForeignKey(
        "user.api.UserResource",
        "owner",
        readonly=True,
    )

In this commit, add a check to ensure the current class of the
ForeignKey is a subclass of Model of django.

Tested manually

Test case:
Before the fix, the new test case failed
image

After the fix, the test case passed:

image

atodorov
atodorov suggested changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good but I think the class check can be made more robust by using the utils function which is also used in other places.

However we also need to update the test suite, see tests/input/func_noerror_foreignkeys.py or another of the files with foreign in the names (feel free to create another file if you want to).

You should be able to declare your model as per the original comment and together with the patch when the test suite executes it shouldn't report any errors.

pylint_django/transforms/foreignkey.py Outdated Show resolved Hide resolved
@qq915522927 qq915522927 force-pushed the bugfix/fk-check-for-django-model branch from 186b47f to b5edc63 Compare September 7, 2021 10:52
atodorov
atodorov suggested changes Sep 9, 2021
View reviewed changes
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, I've added some comments.

Sadly I think Travis CI has exhausted to quota for this repository and will not execute the tests. Maybe it's time to switch to GitHub Actions.

pylint_django/tests/input/func_noerror_foreign_key_in_non_django_class.py Outdated Show resolved Hide resolved
pylint_django/tests/input/func_noerror_foreign_key_in_non_django_class.py Outdated Show resolved Hide resolved
pylint_django/transforms/foreignkey.py Outdated Show resolved Hide resolved
@qq915522927 qq915522927 force-pushed the bugfix/fk-check-for-django-model branch from b5edc63 to 5b98791 Compare November 5, 2021 10:15
atodorov
atodorov suggested changes Nov 7, 2021
View reviewed changes
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change requested.

pylint_django/tests/input/func_noerror_foreign_key_in_non_django_class.py Outdated Show resolved Hide resolved
Make the ForeiginKey detection more accurate
492454d 
To handle this case: the project use tastypie and django.
tastypie has a `ForeignKey` field which has the same name
as django's `ForeignKey`.
The issue is the lint trys resolving the `ForeignKey` for the
tastypie `ForeignKey` which cause import error.

In this commit, add a check to ensure the current class of the
`ForeignKey` is a subclass of `Model` of django.

Tested manually
Test case added: func_noerror_foreign_key_in_non_django_class
@qq915522927 qq915522927 force-pushed the bugfix/fk-check-for-django-model branch from 5b98791 to 492454d Compare November 9, 2021 06:40
@qq915522927
Copy link
Author

Hi Alexander @atodorov , could this commit be merged?

@atodorov
Copy link
Contributor

Hi Alexander @atodorov , could this commit be merged?

Not before #336, otherwise we have no indication of the testing status. I'm having troubles getting tox to work inside GitHub Actions, feel free to open another PR if you can spot my mistakes.

@carlio
Copy link
Collaborator

carlio commented Dec 28, 2021

@atodorov now that #336 (via #340) is merged, is this PR still good? After conflicts and changes are fixed that is - @qq915522927 if you have time to rebase given the recent changes that'd be great.

@atodorov
Copy link
Contributor

@atodorov now that #336 (via #340) is merged, is this PR still good?

We need the code rebased & the conflicts resolved so we can see a passing result from CI. If CI passes then +1 for merging.

@carlio carlio mentioned this pull request Jan 2, 2022
Merged
carlio added a commit that referenced this pull request Jan 2, 2022
@carlio
Merge pull request #345 from PyCQA/bugfix/fk-check-for-django-model
54618f4 
Rebasing #330
@carlio
Copy link
Collaborator

carlio commented Jan 2, 2022

I pulled the fork branch and fixed the conflicts and rebased in #345 . Tests all passed so I have merged it to master, and can close this PR now.

@carlio carlio closed this Jan 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atodorov atodorov atodorov requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qq915522927 @atodorov @carlio