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

Add function based view pylint checker #734 #955

Closed

Conversation

Projects
None yet
4 participants
@kernel-panic96
Copy link
Contributor

commented May 29, 2019

Unfortunately I could not plug it into the tcms project, I have problems with the python virtual environment. I have created a demo django project to showcase the plugin.

@atodorov
Copy link
Member

left a comment

POC looks good but needs more work. Detection of FBV is kind of flaky ATM.

Show resolved Hide resolved kiwi_lint/__init__.py Outdated
Show resolved Hide resolved kiwi_lint/__init__.py Outdated
Show resolved Hide resolved kiwi_lint/function_based_views.py Outdated
Show resolved Hide resolved kiwi_lint/function_based_views.py Outdated
Show resolved Hide resolved kiwi_lint/function_based_views.py Outdated

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.views_by_module = self.group_views_by_module(self.url_mapping.values())

This comment has been minimized.

Copy link
@atodorov

atodorov May 29, 2019

Member

Here I am getting wrong results:

['tcms.core', 'tcms.kiwi_auth', 'tcms.core.contrib.comments.apps.AppConfig', 'tcms.core.contrib.linkreference', 'tcms.management', 'tcms.testcases.apps.AppConfig', 'tcms.testplans.apps.AppConfig', 'tcms.testruns.apps.AppConfig', 'tcms.telemetry', 'tcms.xmlrpc', 'tcms.telemetry.tests.plugin']

^^^ this is from the left-over print statement above, notice the apps which are installed.

*** views by module
{'tcms.core.admin': {'delete_view', 'add_view'},
 'tcms.core.ajax': {'UpdateTestCaseActorsView',
                    'comment_case_runs',
                    'tags',
                    'update_bugs_to_caseruns'},
 'tcms.core.contrib.comments.views': {'delete', 'post'},
 'tcms.core.contrib.linkreference.views': {'add', 'remove'},
 'tcms.core.views': {'dashboard', 'navigation'},
 'tcms.kiwi_auth.admin': {'delete_view', 'user_change_password'},
 'tcms.kiwi_auth.views': {'LoginViewWithCustomTemplate',
                          'PasswordResetView',
                          'confirm',
                          'profile',
                          'register'},
 'tcms.telemetry.tests.plugin.views': {'example'},
 'tcms.telemetry.views': {'TestingBreakdownView'}}

^^^ this is from a print I've added here. Notice how you have both class based and function based views in the list and also how you are missing every app which is declared not by its module name but via its AppConfig.

This comment has been minimized.

Copy link
@kernel-panic96

kernel-panic96 May 29, 2019

Author Contributor

That was my original intention. The DjangoViewsVisiter class only gathers views which are directly or transitively included in the project's urlpatterns, the idea was that by doing it so you could find views which are not listed in a standard 'views.py' file, e.g. tcms.kiwi_auth.admin's views. If your idea was different, I'll be happy to change the implementation.

If you don't see an module in the self.views_by_module (I'm assuming that this was what you printed out), then the module did not contain views who are routed in the project's urlpatterns. Can you elaborate on the last AppConfig, are you reffering to this?

This comment has been minimized.

Copy link
@kernel-panic96

kernel-panic96 May 29, 2019

Author Contributor

I committed a change, that should be able to deal with AppConfig's. Could you please verify the behaviour on the project, I still have issues running it.

This comment has been minimized.

Copy link
@atodorov

atodorov May 29, 2019

Member

With the latest update I have the following views by module:

{'tcms.core.admin': {'add_view', 'delete_view'},
 'tcms.core.ajax': {'UpdateTestCaseActorsView',
                    'comment_case_runs',
                    'tags',
                    'update_bugs_to_caseruns'},
 'tcms.core.contrib.comments.views': {'delete', 'post'},
 'tcms.core.contrib.linkreference.views': {'add', 'remove'},
 'tcms.core.views': {'navigation', 'dashboard'},
 'tcms.kiwi_auth.admin': {'user_change_password', 'delete_view'},
 'tcms.kiwi_auth.views': {'LoginViewWithCustomTemplate',
                          'PasswordResetView',
                          'confirm',
                          'profile',
                          'register'},
 'tcms.telemetry.tests.plugin.views': {'example'},
 'tcms.telemetry.views': {'TestingBreakdownView'},
 'tcms.testcases.admin': {'add_view', 'change_view'},
 'tcms.testcases.views': {'NewCaseView',
                          'SimpleTestCaseView',
                          'TestCaseExecutionDetailPanelView',
                          'attachment',
                          'clone',
                          'edit',
                          'get',
                          'list_all',
                          'load_more_cases',
                          'printable',
                          'search'},
 'tcms.testplans.admin': {'add_view', 'change_view'},
 'tcms.testplans.views': {'DeleteCasesView',
                          'LinkCasesSearchView',
                          'LinkCasesView',
                          'NewTestPlanView',
                          'ReorderCasesView',
                          'UpdateParentView',
                          'attachment',
                          'clone',
                          'edit',
                          'get',
                          'get_all',
                          'printable',
                          'search'},
 'tcms.testruns.admin': {'add_view', 'change_view', 'delete_view'},
 'tcms.testruns.views': {'AddCasesToRunView',
                         'TestRunReportView',
                         'UpdateAssigneeView',
                         'UpdateCaseRunStatusView',
                         'cc',
                         'change_status',
                         'clone',
                         'edit',
                         'get',
                         'new',
                         'remove_execution',
                         'search',
                         'update_case_run_text'}}

Issues:

  • admin.py is not included directly in urlpatterns and it doesn't define any views so being present is wrong.
  • How did 'tcms.telemetry.tests.plugin.views': {'example'}, get included? It is not referenced from tcms/urls.py ?

@kernel-panic96 kernel-panic96 force-pushed the kernel-panic96:function-based-view-lint branch from 958ee0a to 612a3e4 May 29, 2019

kernel-panic96 added some commits May 29, 2019

Show resolved Hide resolved kiwi_lint/function_based_views.py Outdated

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.views_by_module = self.group_views_by_module(self.url_mapping.values())

This comment has been minimized.

Copy link
@atodorov

atodorov May 29, 2019

Member

With the latest update I have the following views by module:

{'tcms.core.admin': {'add_view', 'delete_view'},
 'tcms.core.ajax': {'UpdateTestCaseActorsView',
                    'comment_case_runs',
                    'tags',
                    'update_bugs_to_caseruns'},
 'tcms.core.contrib.comments.views': {'delete', 'post'},
 'tcms.core.contrib.linkreference.views': {'add', 'remove'},
 'tcms.core.views': {'navigation', 'dashboard'},
 'tcms.kiwi_auth.admin': {'user_change_password', 'delete_view'},
 'tcms.kiwi_auth.views': {'LoginViewWithCustomTemplate',
                          'PasswordResetView',
                          'confirm',
                          'profile',
                          'register'},
 'tcms.telemetry.tests.plugin.views': {'example'},
 'tcms.telemetry.views': {'TestingBreakdownView'},
 'tcms.testcases.admin': {'add_view', 'change_view'},
 'tcms.testcases.views': {'NewCaseView',
                          'SimpleTestCaseView',
                          'TestCaseExecutionDetailPanelView',
                          'attachment',
                          'clone',
                          'edit',
                          'get',
                          'list_all',
                          'load_more_cases',
                          'printable',
                          'search'},
 'tcms.testplans.admin': {'add_view', 'change_view'},
 'tcms.testplans.views': {'DeleteCasesView',
                          'LinkCasesSearchView',
                          'LinkCasesView',
                          'NewTestPlanView',
                          'ReorderCasesView',
                          'UpdateParentView',
                          'attachment',
                          'clone',
                          'edit',
                          'get',
                          'get_all',
                          'printable',
                          'search'},
 'tcms.testruns.admin': {'add_view', 'change_view', 'delete_view'},
 'tcms.testruns.views': {'AddCasesToRunView',
                         'TestRunReportView',
                         'UpdateAssigneeView',
                         'UpdateCaseRunStatusView',
                         'cc',
                         'change_status',
                         'clone',
                         'edit',
                         'get',
                         'new',
                         'remove_execution',
                         'search',
                         'update_case_run_text'}}

Issues:

  • admin.py is not included directly in urlpatterns and it doesn't define any views so being present is wrong.
  • How did 'tcms.telemetry.tests.plugin.views': {'example'}, get included? It is not referenced from tcms/urls.py ?
Show resolved Hide resolved kiwi_lint/function_based_views.py Outdated
Show resolved Hide resolved kiwi_lint/function_based_views.py Outdated
Show resolved Hide resolved kiwi_lint/__init__.py Outdated
@codecov-io

This comment has been minimized.

Copy link

commented May 29, 2019

Codecov Report

Merging #955 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #955   +/-   ##
=======================================
  Coverage   73.15%   73.15%           
=======================================
  Files         112      112           
  Lines        4764     4764           
  Branches      609      609           
=======================================
  Hits         3485     3485           
  Misses       1057     1057           
  Partials      222      222

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67588c8...e330014. Read the comment docs.

@coveralls

This comment has been minimized.

Copy link

commented May 29, 2019

Pull Request Test Coverage Report for Build 3267

  • 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 73.789%

Totals Coverage Status
Change from base Build 3265: 0.0%
Covered Lines: 3707
Relevant Lines: 4764

💛 - Coveralls

@kernel-panic96 kernel-panic96 force-pushed the kernel-panic96:function-based-view-lint branch from 5dba100 to e330014 Jun 1, 2019

@atodorov
Copy link
Member

left a comment

It is getting there, now functions almost fine but needs a few more adjustments.

@@ -52,7 +52,7 @@ check: flake8 test
.PHONY: pylint
pylint:
pylint -d missing-docstring *.py kiwi_lint/
PYTHONPATH=.:./tcms/ pylint --load-plugins=pylint_django --load-plugins=kiwi_lint -d missing-docstring -d duplicate-code tcms/
PYTHONPATH=.:./tcms/ DJANGO_SETTINGS_MODULE=$(DJANGO_SETTINGS_MODULE) pylint --load-plugins=pylint_django --load-plugins=kiwi_lint -d missing-docstring -d duplicate-code tcms/

This comment has been minimized.

Copy link
@atodorov

atodorov Jun 4, 2019

Member

Revert this change and add -d non-function-based-view-required to it!

Then add the following line:

 PYTHONPATH=. DJANGO_SETTINGS_MODULE=$(DJANGO_SETTINGS_MODULE) pylint --load-plugins=pylint_django --load-plugins=kiwi_lint -d all -e  non-function-based-view-required tcms/

Effectively we want pylint to execute twice, once without the new checker and once with the new checker enabled. This will resolve the import problem with the xmlrpc module.


@classmethod
def _get_url_view_mapping(cls, root_urlpatterns):
def go(urlpatterns, prefix='^', result=None):

This comment has been minimized.

Copy link
@atodorov

atodorov Jun 4, 2019

Member

Name must be longer and more descriptive. Both CodeClimate and pylint complain about it.

return go(root_urlpatterns)

def visit_module(self, module):
self.view_module = module.name if module.name in self.view_files else None

This comment has been minimized.

Copy link
@atodorov

atodorov Jun 4, 2019

Member

Skip modules ending in .admin, ether here or when scanning urlpatterns.

if isinstance(url, URLPattern):
# path('someurl', view), meaning this is leaf node url
url_pattern = prefix + url.pattern.regex.pattern.strip('^')
result[url_pattern] = (url.callback.__module__, url.callback.__name__)

This comment has been minimized.

Copy link
@atodorov

atodorov Jun 4, 2019

Member

result[rl.callback.__module__] = url.callback.__name__ will directly provide the data structure expected by the checker and negate the need for group_views_by_module() below.

def visit_functiondef(self, func_def):
if self.view_module and func_def.name in self.views_by_module[self.view_module]:
self.add_message('non-function-based-view-required', node=func_def)
print(f'\tview name: \'{func_def.name}\'')

This comment has been minimized.

Copy link
@atodorov

atodorov Jun 4, 2019

Member

Remove print.


project_urls = import_module(settings.ROOT_URLCONF)
self.url_mapping = self._get_url_view_mapping(project_urls.urlpatterns)
self.view_files = set(module for module, _ in self.url_mapping.values())

This comment has been minimized.

Copy link
@atodorov

atodorov Jun 4, 2019

Member

with the proposed refactoring of the internal data structure you don't really need this variable, just take self.url_mapping.keys()

@atodorov atodorov closed this Jun 17, 2019

atodorov added a commit that referenced this pull request Jun 17, 2019

Apply code review comments. Refs #955
- execute separately from the rest checkers due to how we
  modify the Python search path and b/c there are other modules
  named 'xmlrpc' which will cause a traceback
- fix pylint warnings for this checker
- rename to better match what we are doing
- conditionally load Django so we don't crash when
  DJANGO_SETTINGS_MODULE is undefined
- and minor changes to fix false-positive reports when internal
  variables were not reset between modules

atodorov added a commit that referenced this pull request Jun 17, 2019

Apply code review comments. Refs #955
- execute separately from the rest checkers due to how we
  modify the Python search path and b/c there are other modules
  named 'xmlrpc' which will cause a traceback
- fix pylint warnings for this checker
- rename to better match what we are doing
- conditionally load Django so we don't crash when
  DJANGO_SETTINGS_MODULE is undefined
- and minor changes to fix false-positive reports when internal
  variables were not reset between modules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.