python-lsp / pylsp-mypy

Mypy plugin for the Python LSP Server.
MIT License
118 stars 35 forks source link

Create new config option 'config_names' #42

Closed hetmankp closed 1 year ago

hetmankp commented 1 year ago

This configuration option allows us to specify additional configuration file names under which the mypy config could be found.

This is intended to be a draft. Happy to work out details further as necessary.

Richardk2n commented 1 year ago

Given that the search for the config file only goes upwards, I can see why people might want this. Can you name examples of people placing the configs in subdirectories? I have never heard of that practice.

However, what you are presenting is somewhat two features merged into one: Custom search paths and custom names. I think we should differentiate those very well, to prevent unexpected outcomes.

hetmankp commented 1 year ago

Given that the search for the config file only goes upwards, I can see why people might want this. Can you name examples of people placing the configs in subdirectories? I have never heard of that practice.

So I think it can be fairly common pattern for many IDEs. For example, Visual Studio Code places workspace settings in a .vscode directory at the root of the workspace, and user settings in ~/.vscode. Eclipse has the .settings directory for project settings. IntelliJ IDEA uses the .idea directory for project settings. Since tools like pylsp-mypy allow users to build their own custom IDEs I don't think this would be out of place.

From personal experience I also find this becomes much more desirable on big projects that pull from upstream maintainers and have many developers working on them simultaneously as it keeps things much neater, even allowing a reasonable place for per developer configs.

And I think this practice is becoming more common in general, for example with Linux distros increasingly encouraging tools to place their configurations in ~/.config rather than having an explosion of hidden files all over users' home directories.

hetmankp commented 1 year ago

However, what you are presenting is somewhat two features merged into one: Custom search paths and custom names. I think we should differentiate those very well, to prevent unexpected outcomes.

Now in terms of separating the features. I think the intention for the config_names setting was meant to be more of a list of additional settings paths, not necessarily just custom base file names (since a path can consist of both directory and file names). If you think the name of the setting doesn't make that clear perhaps a better name could be picked.

However, it feels like trying to separate these into two separate "base config file name" and "relative config directory" settings is actually going to make the scheme more confusing in terms of explaining the usage patterns to users of the plugin. It would also complicate the implementation code since we already get directories included for free.

That's my personal opinion, however I don't mind how it's represented to the user. What's important for me is that as the plugin searches up the directory tree for configurations that it can check not only custom files names but that those file names can live in custom sub-directories.

Richardk2n commented 1 year ago

I see. I thought of “custom config file name” and “custom config path”. I did not realize you actually did intend on applying the path on higher up directories (though that was an unintended side effect of custom config paths). I am not sure if this is a good idea, because that could make it somewhat confusing which files match. (This is open for debate though) Would specifying a (relative) path from the working directory not work for your use case?

hetmankp commented 1 year ago

@Richardk2n : Yes, I did intend to check for these custom paths while directories are scanned all the way up. If putting the configuration in a subdirectory inside the workspace makes sense, I think it also makes sense for this to be possible higher up (for example once you reach the user's home directory). Putting configurations inside subdirectories does appear to be increasingly encouraged.

I think as long as this behaviour is well documented it won't be particularly confusing, though ultimately you'll have to decide if you're comfortable with that or not.

I need to make sure I understand what you mean by working directory. Do you mean specifying a custom config file path relative to the workspace root? I think this would be enough for what I'm currently doing (though I have to admit I don't know how pylsp actually determines the location of the workspace root, and it does feel a bit less flexible). Additionally it would be nice if one could specify a custom search path relative to the user's HOME directory as well, though just having support for the various default search locations that mypy itself provides would be sufficient.

Let me know how you'd like to proceed and I can work on the code if you like.

Richardk2n commented 1 year ago

Just for clarity: You do not care about being able to change the name, what you care about is the path?

Richardk2n commented 1 year ago

I renamed the feature and took the liberty to change some implementation details.

Does this work for you?

hetmankp commented 1 year ago

I renamed the feature and took the liberty to change some implementation details.

Does this work for you?

I looked over the code and I think this should be sufficient to cover my use case. I won't get a chance to test it out for a couple of weeks though but I'll report back when I do.