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

WIP: Read from stdin #1189

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@cpitclaudel

This is obviously not ready for merging; I just wanted to explore a potential implementation for GH-1187.

This PR makes it possible to pass "-" as a filename to pylint, along with a --stdin-file-name <fname> argument. Everything should happen as if whatever pylint gets from stdin was exactly the contents of <fname>.

I'd like some help to understand:

  • Whether this is the "right" approach
  • What more would be needed to consider merging this

Parts of this should probably go into astroid, too.

WIP: Read from stdin
Connects to GH-1187

@cpitclaudel cpitclaudel referenced this pull request in flycheck/flycheck Feb 5, 2017

Closed

Unable to import 'flycheck_A.B' #1203

+
+from io import StringIO, TextIOWrapper
+if sys.version_info >= (3, 0):
+ def _read_stdin():

This comment has been minimized.

@fabioz

fabioz Mar 28, 2017

For both versions, I think it should read the binary contents and then read the first 2 lines (still in binary mode) and try to match it with the regexp described in: https://www.python.org/dev/peps/pep-0263/ for the declared encoding.

On Python 3, if the encoding is not declared, utf-8 can be considered the default thought (on Python 2 it should go for ascii if it's not declared).

@fabioz

fabioz Mar 28, 2017

For both versions, I think it should read the binary contents and then read the first 2 lines (still in binary mode) and try to match it with the regexp described in: https://www.python.org/dev/peps/pep-0263/ for the declared encoding.

On Python 3, if the encoding is not declared, utf-8 can be considered the default thought (on Python 2 it should go for ascii if it's not declared).

@fabioz

This comment has been minimized.

Show comment
Hide comment
@fabioz

fabioz Mar 28, 2017

This would be really nice to integrate into IDEs better (i.e.: to do a code-analysis with PyLint having dirty contents inside the IDE).

One thing I noticed is that the pull request has no tests (so, probably a blocker for commit).

Unfortunately, I can't really comment on the general approach -- I'm not really that familiar with the PyLint codebase ;)

fabioz commented Mar 28, 2017

This would be really nice to integrate into IDEs better (i.e.: to do a code-analysis with PyLint having dirty contents inside the IDE).

One thing I noticed is that the pull request has no tests (so, probably a blocker for commit).

Unfortunately, I can't really comment on the general approach -- I'm not really that familiar with the PyLint codebase ;)

@cpitclaudel

This comment has been minimized.

Show comment
Hide comment
@cpitclaudel

cpitclaudel Mar 28, 2017

Thanks for taking a look! Should be fairly easy to reuse most of the integration tests (by feeding files on stdin). But I didn't want to start doing that until I got at least some word from the Pylint devs. The trick is, they said it would be hard to do when I asked for the feature, so I thought I'd provide a draft to start a discussion :)

Thanks for taking a look! Should be fairly easy to reuse most of the integration tests (by feeding files on stdin). But I didn't want to start doing that until I got at least some word from the Pylint devs. The trick is, they said it would be hard to do when I asked for the feature, so I thought I'd provide a draft to start a discussion :)

@fabioz

This comment has been minimized.

Show comment
Hide comment
@fabioz

fabioz Mar 28, 2017

Well, I'm not a PyLint dev, but maybe I can help spur some discussion...

I think the best approach would be making it more targeted (there should be a single place which loads a file content and converts it to an AST -- i.e.: MANAGER.ast_from_file(filepath, modname, source=True))

I think that I'd just create some config in the related library which lets you create a dict(filepath->override contents) and set that when stdin with a filepath is chosen (without touching any other related code).

This probably happens on astroid, so, I'd start getting that feature (preset file contents) there and then coming to PyLint to use it.

Also, you probably need to take some care to always access the keys on that dict by real/abs paths (so that if it's accessed by 2 places from symlinks the same overridden contents are used).

fabioz commented Mar 28, 2017

Well, I'm not a PyLint dev, but maybe I can help spur some discussion...

I think the best approach would be making it more targeted (there should be a single place which loads a file content and converts it to an AST -- i.e.: MANAGER.ast_from_file(filepath, modname, source=True))

I think that I'd just create some config in the related library which lets you create a dict(filepath->override contents) and set that when stdin with a filepath is chosen (without touching any other related code).

This probably happens on astroid, so, I'd start getting that feature (preset file contents) there and then coming to PyLint to use it.

Also, you probably need to take some care to always access the keys on that dict by real/abs paths (so that if it's accessed by 2 places from symlinks the same overridden contents are used).

@cpitclaudel

This comment has been minimized.

Show comment
Hide comment
@cpitclaudel

cpitclaudel Mar 28, 2017

I think that's pretty close to what the code already does (it calls that a proxy, which allows you to decouple what file you claim to be checking and where you read from. The where is only stdin in my patch, but that would be easy to extend.

The current strategy requires adding just two functions to Astroid, which sounds like a feature. In particular, the notion of proxying doesn't need to exist there.

But all in all, I don't think I'm going to write more code for this until I get at least a sign of life from the Pylint devs :) This is almost 4 months old, so I don't want to invest more time without knowing if it'll ever pay off.

I think that's pretty close to what the code already does (it calls that a proxy, which allows you to decouple what file you claim to be checking and where you read from. The where is only stdin in my patch, but that would be easy to extend.

The current strategy requires adding just two functions to Astroid, which sounds like a feature. In particular, the notion of proxying doesn't need to exist there.

But all in all, I don't think I'm going to write more code for this until I get at least a sign of life from the Pylint devs :) This is almost 4 months old, so I don't want to invest more time without knowing if it'll ever pay off.

@dakra

This comment has been minimized.

Show comment
Hide comment
@dakra

dakra Apr 13, 2017

Is there any news on this? The PR is almost half a year old and a pylint dev has yet to comment.

Would be really nice to see good integration in editors.

dakra commented Apr 13, 2017

Is there any news on this? The PR is almost half a year old and a pylint dev has yet to comment.

Would be really nice to see good integration in editors.

@PCManticore

This comment has been minimized.

Show comment
Hide comment
@PCManticore

PCManticore Apr 13, 2017

Member

Hi folks,

Sorry for coming so late commenting on this issue.
I would love to have this feature, especially if it helps IDEs better integrate pylint. Since this is just a draft, here's my idea of how this patch should look:

  • we shouldn't touch expand_files at all. Ideally, pylint should use two modes, the current one over files and another one over stdin. We should check if stdin analysis was requested. To accomplish, probably ``_do_check``` has to refactored, the block that handles the files should be a separate function, with another one that builds the AST from the stdin string.
  • since we already have a file mode, we don't need the stdin file name to be anything else than a flag, which should say that the analysis should either use files (and if no files are passed, then it should fail with the help message as it currently those) or if it should use stdin.
  • there is no need to support this with multiple jobs.
  • nothing needs to be changed in astroid. Whenever we use stdin, we just use string_build, this should be sufficient.
  • other than these, it will need tests to ensure it does the right job, the rest is just boilerplate (Changelog, what's new entries etc)
Member

PCManticore commented Apr 13, 2017

Hi folks,

Sorry for coming so late commenting on this issue.
I would love to have this feature, especially if it helps IDEs better integrate pylint. Since this is just a draft, here's my idea of how this patch should look:

  • we shouldn't touch expand_files at all. Ideally, pylint should use two modes, the current one over files and another one over stdin. We should check if stdin analysis was requested. To accomplish, probably ``_do_check``` has to refactored, the block that handles the files should be a separate function, with another one that builds the AST from the stdin string.
  • since we already have a file mode, we don't need the stdin file name to be anything else than a flag, which should say that the analysis should either use files (and if no files are passed, then it should fail with the help message as it currently those) or if it should use stdin.
  • there is no need to support this with multiple jobs.
  • nothing needs to be changed in astroid. Whenever we use stdin, we just use string_build, this should be sufficient.
  • other than these, it will need tests to ensure it does the right job, the rest is just boilerplate (Changelog, what's new entries etc)
@PCManticore

This comment has been minimized.

Show comment
Hide comment
@PCManticore

PCManticore Jan 30, 2018

Member

Closing this as it is very old.

Member

PCManticore commented Jan 30, 2018

Closing this as it is very old.

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