-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
write lockfile using tomli-w #9637
Conversation
b49c507
to
69b4ef2
Compare
I am still slightly -1 on this. I think it's not worth introducing another dependency, no matter how small it is, because afaik we do not have major issues with tomlkit for writing the lock file. Of course, the code is a bit simpler but only because we did some optional explicit formatting with tomlkit. If we did not want the explicit formatting - which I do not suggest - the code with tomlkit would be almost as simple as with tomli-w. |
For me it's more about: Not so much that this does or doesn't make poetry itself simpler, but that However I won't be losing any sleep either way. |
I second @radoering, -1 from me. I don't see a penalty of new dependency and (IMHO uglier) formatting worth it. |
Of course the trigger for this conversation is that it is not working as users would wish, and so #9468 is introducing some ugly workaround. I agree that tomlkit is mostly working, and that without a way to remove it entirely, it is awkward to remove it only partially. (I notice that only the other day they fixed yet another bug with out-of-order tables - though that sort of thing is more relevant to pyproject.toml than the lock file. A better tomlkit is definitely on my wishlist for the python ecosystem!) |
However, that is not the fault of tomlkit but stems from our unconventional usage (reading with tomli and writing with tomlkit). I would not call the additional code a workaround but a performance optimization. (The original proposal of the author of #9468 used tomlkit as designed. It is just a bit slow.) I think about a dozen lines of code (+ comments), which we probably do not have to touch often (famous last words 😉), in exchange for 0.7 s is ok. Further, I think #9468 is not a strong argument because there may be other users who like to keep Long story short, I will close this PR and accept the alternative although it introduces a bit of complexity. Even though the decision is not clear as glass for me, I think the improvement in behavior justifies the added complexity. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
see #9468 (comment)
in view of previous discussions I expect you may turn this down: but it was easy to do and it is good to make the comparison concrete.
test script updates do not show much change, the update to
poetry.lock
is probably more representative.fwiw I mildly prefer what tomli-w is doing with extras, mildly un-prefer what it is doing with files/hashes - and weigh both of these as being less important than freeing poetry of tomlkit quirks wherever possible.