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

Add a decorator to warn on wrong file extension #2573

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add a decorator to warn on wrong file extension #2573

wants to merge 1 commit into from

Conversation

jeandet
Copy link
Member

@jeandet jeandet commented Nov 2, 2017

Any module can use this decorator to warn the user if he passes
wrong files to the method based on their extension.
This reimplements #2565 in a more generic way.

ignatenkobrain
ignatenkobrain reviewed Nov 4, 2017
View reviewed changes
mesonbuild/interpreterbase.py Outdated
@@ -55,6 +55,37 @@ def wrapped(self, node, args, kwargs):
return f(self, node, args, kwargs)
return wrapped

#This decorator does the same than permittedKwargs plus:
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

block comment should start with '# '

ignatenkobrain
ignatenkobrain reviewed Nov 4, 2017
View reviewed changes
mesonbuild/interpreterbase.py Outdated
for key, item in kwargs.items():
if key in self.extensions_dict:
extensions = self.extensions_dict[key]
if not extensions is None:
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

test for object identity should be 'is not'

ignatenkobrain
ignatenkobrain reviewed Nov 4, 2017
View reviewed changes
mesonbuild/mesonlib.py Outdated
if hasattr(s, 'held_object') or isinstance(s, File):
pass
elif isinstance(s, str):
# self.validate_within_subproject(self.subdir, s)
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

indentation is not a multiple of four (comment)

ignatenkobrain
ignatenkobrain reviewed Nov 4, 2017
View reviewed changes
mesonbuild/modules/qt.py Outdated
@@ -19,7 +19,7 @@
from ..dependencies import Qt4Dependency, Qt5Dependency
import xml.etree.ElementTree as ET
from . import ModuleReturnValue, get_include_args
from ..interpreterbase import permittedKwargs
from ..interpreterbase import permittedKwargs, processArgs
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

'..interpreterbase.permittedKwargs' imported but unused

@jeandet jeandet force-pushed the add_file_extension_checker_decorator branch 2 times, most recently from 53af3c8 to 6916f03 Compare November 4, 2017 13:36
@jeandet jeandet mentioned this pull request Nov 4, 2017
Closed
@jeandet
Copy link
Member Author

jeandet commented Nov 4, 2017

I've also added string to File conversion in this decorator, this would help modules to use File everywhere.

@jeandet jeandet added the WIP label Nov 5, 2017
@ignatenkobrain
Copy link
Member

Flake8 detected 9 issues on ae3f246
Visit https://sideci.com/gh/19784232/pull_requests/2573 to review the issues.

@jeandet jeandet force-pushed the add_file_extension_checker_decorator branch 2 times, most recently from a72adcf to 6dbcdbf Compare November 12, 2017 22:38
@jeandet jeandet force-pushed the add_file_extension_checker_decorator branch from 6dbcdbf to f1f3b4a Compare November 13, 2017 18:58
@jeandet jeandet removed the WIP label Nov 13, 2017
@jpakkane
Copy link
Member

I don't like the fact that this reduces readability. permitted_kwargs is unambiguous, whereas process_kwargs is not. It would be better if the old decorator remained and this functionality were added in a new decorator, say check_extensions or warn_extensions that does only this one thing.

Decorators can be stacked on top of each other without problems, see for example interpreter.py where we do a lot of it.

@jeandet
Copy link
Member Author

jeandet commented Nov 13, 2017

I agree the name sucks. But stacking decorators has few drawbacks:

  • This function also silently convert string lists to list of files
  • Decorators call uses many lines to list args while it does almost nothing
  • Makes permitted_kwargs and check_extensions optional where I believe that both should always be used
  • Adding a type checking for kwargs might lead to another decorator with a big argument list
    So my first commit was using two decorators but it makes a lot of code duplication and I think kwargs validation should be done in one call.

@jeandet jeandet added the WIP label Nov 16, 2017
@jeandet
Add a decorator to warn on wrong file extension
33a5352 
Any module can use this decorator to warn the user if he passes
wrong files to the method based on their extension.

source_strings_to_files, validate_within_subproject and
evaluate_subproject_info are now part of mesonlib.
@jeandet jeandet force-pushed the add_file_extension_checker_decorator branch from f1f3b4a to 33a5352 Compare November 16, 2017 22:41
@leiflm
Copy link
Contributor

leiflm commented Nov 20, 2017

  • This function also silently convert string lists to list of files

see #2438 (comment)

I'm not a python ninja, but let me know if i can help out somehow.

@nirbheek nirbheek mentioned this pull request Jun 3, 2018
Closed
@nirbheek nirbheek marked this pull request as draft August 13, 2020 17:41
@nirbheek nirbheek removed the WIP label Aug 13, 2020
@eli-schwartz
Copy link
Member

I guess if this is still desirable, it should be redone to be an attribute of a KwargInfo object?

@dcbaker
Copy link
Member

dcbaker commented Dec 15, 2021

Yeah, probably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ignatenkobrain ignatenkobrain ignatenkobrain left review comments

@nirbheek nirbheek Awaiting requested review from nirbheek

@jpakkane jpakkane Awaiting requested review from jpakkane jpakkane is a code owner

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

Successfully merging this pull request may close these issues.
7 participants
@jeandet @ignatenkobrain @jpakkane @leiflm @eli-schwartz @dcbaker @nirbheek