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

permit the user to specify module's name together with the file path #881

Closed
wants to merge 1 commit into from

Conversation

horta
Copy link

@horta horta commented Apr 23, 2016

This would solve the relative import problem happening with linter-pylint of Atom editor: AtomLinter/linter-pylint#110

The trouble is that linter-pylint copy the file to be analyzed to a temporary folder for which pylint/astroid infer as being a standalone script, whereas it is actually a module inside a package.

Pipeline
  1. Users wants to lint the file /PROJECT/PYPACKAGE/PYMODULE.py
  2. linter-pylint copies it to a temporary folder (to properly support linting while the user is still typing some code, I suppose).
  3. linter-pylint would then call: pylint PYPACKAGE.PYMODULE:/tmp/PYMODULE.py

Thereby preventing astroid to guess PYMODULE's full name.

@@ -810,7 +815,9 @@ def expand_modules(files_or_modules, black_list, black_list_re):
result = []
errors = []
for something in files_or_modules:
if exists(something):
if _path_delimiter() in something:
modname, filepath = something.split(':')
Copy link

Choose a reason for hiding this comment

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

@horta I think .split should also be invoked with path_delimiter instead of :

@PCManticore
Copy link
Contributor

Left a comment. While I don't necessarily like the approach linter-pylint is having and thinking that this fix shouldn't be here in pylint, but rather in their framework, I don't mind bringing this in if it fixes the problem you're having, as long as we're describing in clear words that it is a "hack" and it shouldn't be relied upon as a real API. I'll merge it as soon as it's ready. Let me know if it indeed works for your Atom use case, I'm not an user of it.

@horta
Copy link
Author

horta commented Apr 25, 2016

Thank you guys. This first commit was more of a proposal. I will finish that by tonight and add help messages for the user (and hack comments). Thank you!

@horta
Copy link
Author

horta commented Apr 25, 2016

I'm not sure if this can be understood as a "hack". As far as I know, full module names and file paths are loosely related in Python (Please, correct me if I'm wrong). In which case what pylint does via Astroid to infer file's full module name is really a guess.

@@ -54,6 +46,12 @@


MANAGER = astroid.MANAGER
USAGE = __doc__ + """
Argument:
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be documented. From my point of view, this is the hack we're supporting for linter-atom's sake.

@PCManticore
Copy link
Contributor

This is a hack, the semantic of the "path" we're supporting with this patch does not exist at the Python's level and it is added for linter-pylint's sake. Please the remove the documentation changes about this, it's not something users should rely upon.

@horta
Copy link
Author

horta commented Apr 27, 2016

@PCManticore I think I might have found a better solution that would not require changing pylint. I read about namespace packages which allows directories from different package package hierachy to be combine in a single package (i.e. namespace package). I will see if that can be simulated, in which case a fix on linter-pylint would be enough!

I will report it back. Thanks!

@horta
Copy link
Author

horta commented Apr 29, 2016

For who is interested, I proposed a fix for linter-pylint itself since the problem is not really with pylint. AtomLinter/linter-pylint#140

@PCManticore
Copy link
Contributor

Thank you, @horta. FYI though, we don't support namespace packages yet, there is an open issue to add support for it in pylint. I'm closing this PR in the mean time, if you sitll need it after your patch to linter-pylint, you can reopen it.

@horta
Copy link
Author

horta commented May 2, 2016

For those interested, I solved my problem by forking linter-pylint to create my own package named linter-py. The solution I implemented is basically duplicating the entire folder hierarchy (via hard links) of the respective Python package and applying pylint to it. Right now it is release as v1.0.0-beta0 and designed for Atom ^1.8.0-beta3. Thank you, again!

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