Skip to content

WIP: Read from stdin #1189

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

Closed
wants to merge 1 commit into from
Closed

Conversation

cpitclaudel
Copy link
Contributor

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.

@PCManticore PCManticore added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Mar 1, 2017

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

Choose a reason for hiding this comment

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

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

Closing this as it is very old.

@twmr twmr mentioned this pull request Feb 13, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs review 🔍 Needs to be reviewed by one or multiple more persons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants