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

write lockfile using tomli-w #9637

Closed

Conversation

dimbleby
Copy link
Contributor

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.

@dimbleby dimbleby mentioned this pull request Aug 17, 2024
2 tasks
@radoering
Copy link
Member

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.

@dimbleby
Copy link
Contributor Author

For me it's more about: tomlkit has long brought more than its share of bugs and issues, and I'd be glad to lose it wherever possible.

Not so much that this does or doesn't make poetry itself simpler, but that tomli-w is enormously simpler thantomlkit.

However I won't be losing any sleep either way.

@Secrus
Copy link
Member

Secrus commented Aug 17, 2024

I second @radoering, -1 from me. I don't see a penalty of new dependency and (IMHO uglier) formatting worth it. tomlkit is far from perfect, but it works and we already have it in place.

@dimbleby
Copy link
Contributor Author

dimbleby commented Aug 17, 2024

but it works

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!)

@radoering
Copy link
Member

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.

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 \r\n on Windows. If I understand correctly we would lose that with tomli-w. In other words, with tomli-w we would solve one issue just to provoke a new issue from the other side. (Personally, I do not think that it is that relevant whether we always use \n or only use \n on Linux and \r\n on Windows because you can configure your .gitattributes to stay consistent over different operating systems but apparently some users care.)

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.

@radoering radoering closed this Aug 18, 2024
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants