Stop after the first installdeps hook succeeds #450
Conversation
|
@obestwalter any feedback on this PR? If this looks good I'd like to also do the same for the virtualenv creation (which is less of a noop and more of a problem :D). |
|
Hi @asottile Disclaimer: I have not worked my way very far into the internals of tox/pluggy yet. I also have not much experience in estimating the potential backwards compatibility breakhages changes like yours can introduce. The little time I was able to spend with tox until now was gathering experience with how other contributors and users use and understand tox. So atm I am mainly an OSS gardener who is trying to tidy up a bit and keeps the weeds back but does not yet understand much about the deeper organisation :) One thing I am pretty sure about: we need a good reason to change the definition of a hook as this is part of the API and other implementers rely on its current behaviour. How many those are I don't know. I also don't know if maybe all implementers of that hook have the same problem like you and you would solve everybodies problems with this change. I had a closer look now anyway and one thing is that this hook is marked experimental, but because of the use of the venv and session objects - see changelog entry:
Now for working towards a decision if we want to change this - I only have questions: Is there another way to reach your goal or is there a strong case for really changing that hook for all users? I have no idea, because I started exactly now to actually think about hooks in tox and their implications. Why has no other tox hook Why was this hook not marked with So ... more questions than answers I am afraid. I definitely don't feel qualified to make an informed decision about the validity of this change. |
|
Correction: |
|
I typed out a really long response to this but I guess I never posted it, here's an attempt to remember what I wrote the first time :) I did some searching around for implementers of this hook:
I tried implementing my hook in another way, but the only possible way I could was to monkeypatch as a side-effect of plugin importing, which seems to defeat the purpose of a plugin system entirely :) I think the argument for Definitely would love to get input from @hpk42 and @brmzkw! (I think this typed up version was better than the first time I typed it up!) |
|
From what I understand, I made the original PR. Can you give me a link to it? I can't find it, and I can't remember having done anything on tox lately. Seeing some code or discussion might refresh my memory and I might be able to give more insights. Thanks, |
|
Ups - sory Julien, I misread the diff in the Changelog adding this - your name was mentioned in the line above ... |
|
Any updates on this? |
|
Bump: 1 more week @hpk42 @obestwalter <3 |
|
@asottile o.k. I let your enthusiasm infect me and will have a go at determining wether merging this will cause the end of the world as we know it for existing users. Expect a reply in the next few days :) |
|
<3, I've also opened #469 such that both can be considered at the same time (if agreeable) |
|
O.k. I had another look and I would be o.k. to merge this and #469 after the next minor release, which will happen soonish. I say after, because I don't want this to block the next release of tox and detox and I still think that @hpk42 should greenlight this, as I don't know the reasoning behind why this hook is not marked that way already and he has way more experience with potentially breaking changes like this. |
|
@hpk42 thoughts? We'd like to start being able to take advantage of this :) |
|
So far the result of |
|
@obestwalter I've tried and failed to contact @hpk42 (I imagine on holiday or such?) both here and irc :( Given there hasn't been any objection to this (and the related change) in the last almost-two months, is this mergeable? I've been waiting quite patiently without action items but this is getting to be quite a long wait time for an "active" project (and a very simple PR). I'd really love to start being able to use these two hooks which are unusable in the current state :) |
|
@asottile thanks for your patience. This project is definitely active without quotes but your PR proposes a change to an existing API (albeit marked experimental), so this is not to be taken lightly. I wanted to give some time for feedback from the old hands here. The way it looks atm, you can expect your PRs to be in the 2.7.0 release next week :) |
|
Didn't mean to call the project inactive :D, I've definitely observed lots of activity and progress! ===== Sweet, for now I'm going to leave out printing / additional behaviour / etc. I think that can be done as a followup -- if a hook wants to print, it can already do so now and if one wants to know whether the original hook ran, the I will spruce up the docs though, they definitely could do with some additional prose for these two hooks. |
|
I've spruced up the docstrings of the hookspecs for these two hooks. Let me know what you think :) |
By changing this hook to
firstresult=True, a plugin may override this step and avoid running the base stepI'd like to write a plugin which overwrites the install_deps command, I've got it mostly working except when I run it it triggers dependency install twice (the second time is a noop, but a slow noop).
tox2.ini
Current behaviour
The important bit here being:
A
-vvinto that (you'll notice the second install is unnecessary).With my patch:
Hook implementation
A bit of a WIP but it mostly does what I want: