wyuenho / emacs-pet

Tracks down the correct Python tooling executables from your virtualenvs so you can glue the binaries to Emacs and delete code in init.el
GNU General Public License v3.0
105 stars 13 forks source link

Allow the location of pyproject.toml/Pipfile etc. to be customized #5

Closed janoc closed 2 years ago

janoc commented 2 years ago

As promised in #4, I have tested the updated version at work - and it does work.

However, the project there is structured differently and unless I move/symlink pyproject.toml to the project root (where .git folder is), then it fails to find it (to be expected).

It would be very helpful to allow to specify additional places where to look for these files (pyproject.toml, Pipfile, etc.). E.g. my work project is structured as follows:

project root
|-- .git
|-- README.md
|-- src 
      |-- pyproject.toml
      |-- main 
           |-- main.py
...

So it would be really helpful if I could specify, whether per project or globally using some variable, that pet should search not only in the project root but also in ${project root}/src.

My other work projects that use Pipenv have a similar problem - the Pipfile is not at the root of the project for organizational reasons - there are multiple subprojects there and only part of them use Python.

wyuenho commented 2 years ago

Ah thanks for testing pet on a more complicated project layout. I knew this day would come when people test it on a monorepo or something, but didn't anticipate it would be this soon lol. I'll have to think about how best to implement this. Let me know if you have any ideas :)

janoc commented 2 years ago

Hehe, well, that repo is actually quite simple still :)

I was thinking it would have been enough to have the pet-find-file-from-project-root function to work similarly to how executable-find works - have a list of candidate directories and look in them in order until the file is found or the list exhausted. The first candidate to check would always be the project root and the values from the list would be tried afterwards if the files are not in the root.

This list could be a customizable setting, so that I can put there e.g. the src subdirectory in it for the project above using .dir-locals on a per-project basis or from my init file.

BTW, I have tested pet on a Pipenv based project and there it has some weird behavior too - it finds pipenv but doesn't use the right virtual env for whatever reason. But our setup could be weird, we are installing python using pyenv and installing pipenv through that python, so maybe too complex to handle within pet.

wyuenho commented 2 years ago

BTW, I have tested pet on a Pipenv based project and there it has some weird behavior too - it finds pipenv but doesn't use the right virtual env for whatever reason. But our setup could be weird, we are installing python using pyenv and installing pipenv through that python, so maybe too complex to handle within pet.

Can you file a separate issue so you can elaborate on it?

janoc commented 2 years ago

Will do, need to do more debugging on it tomorrow to see what is going on there.

wyuenho commented 2 years ago

See if this branch works for you

janoc commented 2 years ago

Hum, I have checked the commit there but I think this will have issues:

pet-find-file-from-project-root-recursively will return first matching file from the project, either using projectile or project.el.

In such situation it should probably ask the user to pick the right file instead of blindly taking the first match. In a monorepo one could perhaps argue that having multiple configurations is a bad idea even though this is common - but the search will also pick up the toml/Pipfile in any .venv subfolders from installed dependencies which may ship their own pyproject.toml unless the user has that folder excluded in their .projectile or .gitignore. That would be bad.

A good idea could be also allowing the user to supply their own search function/hook in addition to these predefined ones - that would allow the user to adapt to whatever crazy project structure they may have without you having to support and deal with all sorts of crazy cases.

Is it possible for me to just prepend my own function to pet-find-file-functions? Or is something going to break if I do that?

janoc commented 2 years ago

I did test the arbitrary-config-file-path branch with pyproject.toml being in a subdirectory and now it does find the virtualenv nicely, at least for a simple dummy project. Nice!

I will test it tomorrow on the more complicated work projects, hopefully it will work properly.

wyuenho commented 2 years ago

pet-find-file-from-project-root-recursively is the last function to try if pet-locate-dominating-file fails. In your case it'll never fail because there's always a pyproject.toml file in a parent directory. But yes you are correct, if this fails, then pet-find-file-from-project-root-recursively will just pick the first one, so if you are on a sibling directory where none of the config files are found up the directory chain, it'll pick the sibling's or descendants'.

It's probably not ideal, that's why pet-find-file-functions is a defcustom :)

janoc commented 2 years ago

I would suggest to put a check there if the function returns more than one value and if there is, use a completing-read to get the user to resolve it. Otherwise they may have an unexpected surprise on their hand.

wyuenho commented 2 years ago

What happens if one doesn't use projectile or project.el?

project.el is builtin, so if you are not using projectile, project.el will be used. It's possible that you are visiting a file that's not in a project (projects are defined by a version controlled directory in project.el, and a directory with some special metadata files like pyproject.yaml and/or vc'ed by projectile), then nothing special happens, pet-executable-find should still be able to find the executables directly from exec-path as if it's just an alias to executable-find. The one case where this logic will probably do the wrong thing is if you are not using projectile and your "project" is not vc'ed.

In that case, the easiest thing to do is to just install projectile or do a git init lol

janoc commented 2 years ago

I don't mind if projectile or project.el are required (I use projectile since ages ago). I would only suggest making that explicit in the README because the current formulation is a bit unclear about this.

wyuenho commented 2 years ago

I would suggest to put a check there if the function returns more than one value and if there is, use a completing-read to get the user to resolve it. Otherwise they may have an unexpected surprise on their hand.

It's not a bad idea, but I'll probably wait til somebody actually finds this an issue. I suspect pet-locate-dominiting-file is going to cover 99% of the cases.

janoc commented 2 years ago

That's probably true, that will likely cover also the .venv "disaster" in most cases.

janoc commented 2 years ago

I have tested the arbitrary-config-file-path branch with my work project using Poetry and subdirectories - and it works great! :+1:

I think I have found the reason why the pipenv code doesn't work, see #6

wyuenho commented 2 years ago

The arbitrary-config-file-path branch has been merged into main.

wyuenho commented 2 years ago

If you are not using projectile or in a vc'ed repo, pet should work better now, meaning pet-executable-find should find the executable either by pyenv which or plain old executable-find.