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

set project_root where the config file may be #53

Closed

Conversation

christopherpickering
Copy link

Set the project root to be where a config file is found, otherwise set it to the project root... if no root, then set to where the file is and let mypy search for the config.

@kaste
Copy link
Collaborator

kaste commented Oct 14, 2022

Well, dear Sir, there seems to be a bit of duplication in the ifs. 😏
Why shouldn't this be part of SublimeLinter core though? You already did that for pylint just 6 minutes ago, approx. And path_upwards is a copy/paste from there too. Heck, I wrote it.

@FichteFoll
Copy link
Collaborator

I'd like to point out that there should be a way for the user to override this behavior.

@christopherpickering
Copy link
Author

@FichteFoll adding the "working_dir" to the settings file seems to override it. Is that what you're referring too?

image

@kaste
Copy link
Collaborator

kaste commented Oct 14, 2022

The ${project_root} is only used for the working dir by default. So overriding working_dir disables the feature. Users can use the context variable in args or wherever they want though. That's at least what we do for node based linters. Of course in the node world a package.json is basically mandatory. There is no equivalent for that in the Python world. (Maybe .git is a good marker for what defines a project.) See https://github.com/dense-analysis/ale/blob/89db85121c001fc60787647f012978a2328816a5/autoload/ale/python.vim#L19-L64 which checks a lot of possible candidates (but not .git). The simple and old requirements.txt comes to mind as well.

But enumerating a lot of possible names just leads to false matches. It might be better to look for few, 100% names and that is surely not .git because we still fall-back to what we currently have if nothing else (or better) can be found. 🤔

@christopherpickering
Copy link
Author

@kaste thanks! for my side, I don't understand what you want me to change? Are you suggesting to remove the .git match line? Personally I think its useful to make the function return faster, even if sublime will do something similar. Its the same pattern logic that isort and black are using to find a project settings file.

image

@kaste
Copy link
Collaborator

kaste commented Oct 17, 2022

I don't suggest anything, I just speak out loud what we're doing here. What are actually the pros and cons? Is it better to have a lot of exists tests here or just a few? And then why? Will this break for people not being me? Is what I think of as a best practice just a new best practice and all old projects and people do not follow...

Why is having ".git" in there faster btw?

Anyway, when you have 6 same conditionals you have a for loop. And you have a class variable probably defining "names" to look for. That's on the implementation side.

@christopherpickering
Copy link
Author

👍🏽 .git is generally at the project root so hitting this will stop and further checks.
Thanks!

kaste added a commit to SublimeLinter/SublimeLinter that referenced this pull request Feb 7, 2023
Fixes #1883
Fixes SublimeLinter/SublimeLinter-pylint#57
Closes SublimeLinter/SublimeLinter-mypy#53
Closes SublimeLinter/SublimeLinter-pylint#66
Ref #1890

- Detect the typical name "venv" as a virtual environment candidate
- Search for typical files/names and set `project_root` accordingly.
  This will in turn affect the `working_dir` we use unless overridden
  by the user.
kaste added a commit to SublimeLinter/SublimeLinter that referenced this pull request Feb 7, 2023
Fixes #1883
Fixes SublimeLinter/SublimeLinter-pylint#57
Closes SublimeLinter/SublimeLinter-mypy#53
Closes SublimeLinter/SublimeLinter-pylint#66
Ref #1890

- Detect the typical name "venv" as a virtual environment candidate
- Search for typical files/names and set `project_root` accordingly.
  This will in turn affect the `working_dir` we use unless overridden
  by the user.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants