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 application-paths option to find local apps #161

Closed
wants to merge 5 commits into from

Conversation

tmshn
Copy link

@tmshn tmshn commented Sep 30, 2018

Currently, we need to write down all application-specific modules in advance of running flake8, which is a bit troublesome.

Here, I introduced a new application-paths option to find application-specific modules.

With this option, you can do

flake8 --application-paths='.'

instead of

flake8 --application-import-names='flake8_import_order,tests'

@tmshn tmshn changed the title Add application-paths option to find local apps [WIP] Add application-paths option to find local apps Sep 30, 2018
@@ -49,7 +49,10 @@ def _load_test_cases():
def _checker(filename, tree, style_entry_point):
options = {
'application_import_names': [
'flake8_import_order', 'namespace.package_b', 'tests',
'namespace.package_b',
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd want separate tests for this. We'd want to make sure the old version works, as well as the new application paths work. I think we'd even want to ensure they do exactly the same thing.

@@ -114,6 +134,11 @@ def _classify_type(self, module):
elif package in STDLIB_NAMES:
return ImportType.STDLIB

# Check if module's path matches any of given application paths
path = get_module_path(module)
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned that the underlying functions from importlib/pkgutil will cause needless performance degradation. Would it make sense to nest that in after verifying that we even have any application_paths to check?

Copy link
Author

Choose a reason for hiding this comment

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

You're right. It seems they try to import parent modules if the target module has sub packages.

I found another solution using pkgutil.iter_modules:

names = [name for _, name, _ in 
         pkgutil.iter_modules(self.options.get('application_paths', []))]

This runs only once and seems no real import happens. I'll try this.

@tmshn tmshn changed the title [WIP] Add application-paths option to find local apps Add application-paths option to find local apps Sep 30, 2018
@sambrightman
Copy link

Is there anything else needed to get this merged & released?

@tmshn
Copy link
Author

tmshn commented Feb 27, 2019

@sigmavirus24 Just ping. How about this???

@sigmavirus24
Copy link
Member

I've never been given the okay to merge things on this repository. That's up to @PyCQA/flake8-import-order-dev

@sambrightman
Copy link

That's a 404 for me - is the team hidden or has the mention got a typo?

@pgjones
Copy link
Member

pgjones commented Mar 3, 2019

Please refer to the discussion in #163.

@pgjones pgjones closed this Mar 3, 2019
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.

None yet

4 participants