sqlalchemyorg / zimports

Python import rewriter
MIT License
75 stars 6 forks source link

Smarter discovery of the configuration file #13

Open schmir opened 4 years ago

schmir commented 4 years ago

At the moment zimports looks for the configuration file in the current directory. That results in zimports giving different results based on the fact if it's able to load the settings from the current directory or not (e.g. for a src based layout zimports src may give different results from cd src; zimports .)

I think what it should do, is try to find setup.cfg for each argument passed in parent directories of that argument (probably stopping when it finds the root of a git/mercurial/.. repo) or a setup.cfg file. Read that file and merge the options with those given on the command line and use that to sort imports.

If you think this way to discover the setup.cfg file is fine, I can contribute a PR.

sigmavirus24 commented 4 years ago

I think what it should do, is try to find setup.cfg for each argument passed in parent directories of that argument (probably stopping when it finds the root of a git/mercurial/.. repo) or a setup.cfg file

That is vaguely Flake8's methodology and I can tell you that that behaviour is absolutely impractical for every tool to implement and maintain in practice. It's also frustratingly fragile. I would expect anyone looking to keep maintenance costs low to reject this

schmir commented 4 years ago

That is vaguely Flake8's methodology and I can tell you that that behaviour is absolutely impractical for every tool to implement and maintain in practice. It's also frustratingly fragile. I would expect anyone looking to keep maintenance costs low to reject this

Can you give some reasoning for the above statements? I fail to see why this would be impractical to implement and I don't see how this would become fragile.

zzzeek commented 4 years ago

FWIW I agree in the general case with what flake8 and black do, and since my own use case for zimports is that it runs in a .pre-commit file alongside those tools it is appropriate that it would work in the way that they do. I'd only ask that we take a look at flake8 and/or black to see what they are taking into account. For the moment I think the setup.cfg file should remain the file we're looking at, however, adding support for pyproject.toml would only be after flake8 does so.

sigmavirus24 commented 4 years ago

Can you give some reasoning for the above statements?

Well every time someone tries to contribute to that area of flake8 they tend to break things in quite subtle ways. Traversing directories until you find something isn't so easy if you don't find that thing. As soon as projects start looking beyond the current path, then people want you to merge configs in sub-directories and find them in XDG directories and in operating system global locations, e.g., /usr/local/etc and then they want ridiculous precedence and merging strategies.

None of that is simple to maintain. I've yet to find a project that does it well. The simplest path for a maintainer is to just not play that game. Keep it simple. Solve the 98% use-case and focus on doing that well. If the 2% grows, re-evaluate.

No is temporary, yes is forever.


adding support for pyproject.toml would only be after flake8 does so.

There's no time in sight for that.

schmir commented 4 years ago

black uses the following code:

@lru_cache()
def find_project_root(srcs: Iterable[str]) -> Path:
    """Return a directory containing .git, .hg, or pyproject.toml.

    That directory can be one of the directories passed in `srcs` or their
    common parent.

    If no directory in the tree contains a marker that would specify it's the
    project root, the root of the file system is returned.
    """
    if not srcs:
        return Path("/").resolve()

    common_base = min(Path(src).resolve() for src in srcs)
    if common_base.is_dir():
        # Append a fake file so `parents` below returns `common_base_dir`, too.
        common_base /= "fake-file"
    for directory in common_base.parents:
        if (directory / ".git").exists():
            return directory

        if (directory / ".hg").is_dir():
            return directory

        if (directory / "pyproject.toml").is_file():
            return directory

    return directory

def find_pyproject_toml(path_search_start: Iterable[str]) -> Optional[str]:
    """Find the absolute filepath to a pyproject.toml if it exists"""
    path_project_root = find_project_root(path_search_start)
    path_pyproject_toml = path_project_root / "pyproject.toml"
    return str(path_pyproject_toml) if path_pyproject_toml.is_file() else None
schmir commented 4 years ago

i.e. they start from a common_base, that depends on the arguments given, and search the parents of that directory. (I consider that a bug, because it means passing additional src files to be formatted to black, may change the formatting).

sigmavirus24 commented 4 years ago

I consider that a bug, because it means passing additional src files to be formatted to black, may change the formatting

Right. So then the community of "style tools" ends up working in conflicting and a confusing way because no one agrees on the "one right way" to do it and now that these tools have it, no one can change it without breaking large swaths of the user base.

zzzeek commented 4 years ago

im still looking around this strange new universe where someone else is arguing against a feature somebody proposes and I'm fairly OK with it. zimports is pretty closely tied to flake8 as that's my use case. so in theory I would like to be able to run zimports from inside a directory tree. its defaults are what I prefer anyway so this issue doesn't affect me much :)

zzzeek commented 4 years ago

I mean I just looked ( i never deal w this as it works fine for me) and we're actually consuming the flake8 config directly. so it should definitely just do what flake8 is doing and it would be extra nice if it could just make use of flake8's config library directly. that might be less work to keep that mainained rather than duplicating the algorithm itself.

sigmavirus24 commented 4 years ago

I mean I just looked ( i never deal w this as it works fine for me) and we're actually consuming the flake8 config directly. so it should definitely just do what flake8 is doing and it would be extra nice if it could just make use of flake8's config library directly. that might be less work to keep that mainained rather than duplicating the algorithm itself.

Yeah, I've thought about splitting a bunch of things out like that into libraries for related tools to use. This is the first I've had external validation of that as a reasonable idea.

schmir commented 4 years ago

Integration with what flake8 is doing would surely be the best solution (e.g. using the flake8 exclude pattern would make a lot of sense).