fix: keep lock file eol

[trim21]

Pull Request Check List

Resolves: https://github.com/python-poetry/poetry/issues/1488 again

• [x] Added tests for changed code.
• [ ] Updated documentation for changed code.

https://github.com/python-poetry/poetry/issues/1488 once be fixed but it's broken again.

appearly TOMLFile need to call read method to get line encoding, otherwise it just use default os eol.

[Secrus]

Could you please make the fix in tomlkit instead? Sounds like it belongs there more than Poetry code.

[trim21]

Could you please make the fix in tomlkit instead? Sounds like it belongs there more than Poetry code.

tomlkit already fixed this, many times, I think.

https://github.com/python-poetry/tomlkit/pulls?q=is%3Apr+line+ending+is%3Aclosed

they even have test for this

[Secrus]

Well, if it was fixed in tomlkit, we wouldn't need this patch, right? For me, this sounds like a workaround at best.

[trim21]

Well, if it was fixed in tomlkit, we wouldn't need this patch, right? For me, this sounds like a workaround at best.

Actually, I'm not sure if this is a bug of tomlkit, or poetry didn't use it as expected. I guess tomlkit would expect users to read a toml file with TomlFile then write to it， but we are actually reading lockfile with tomllib instead of TomlFile provided by tomlkit

（and personally I hope we just use Unix eol everywhere…）

[trim21]

but you are right, this can be fiexd on both side. tomlkit could get line ending when TomlFile object is created.

[trim21]

/ping

[Secrus]

I still believe that this should be fixed in tomlkit not here

[radoering]

I still believe that this should be fixed in tomlkit not here

I do not think that is possible because since #6562 we use tomli to read the lock file so that tomlkit cannot know what type of line endings are in the lock file. If we want to keep existing line endings we either have to read with tomlkit (what this PR is doing) or determine the type of line ending by ourself and somehow set it in tomlkit. I think I would prefer reading with tomlkit (as it is done in this PR) if it was not a performance issue.

#6562 suggests that there is a relevant performance impact:

Even for a modestly sized project like poetry itself, reading poetry.lock on my laptop takes 0.6 - 0.7 seconds (timings obtained with some time.time() statements inserted into the codebase). tomli manages it in ~0.02 seconds.

Thus, maybe we should really check the timings and investigate the alternative approach.

[trim21]

I still believe that this should be fixed in tomlkit not here

I do not think that is possible because since #6562 we use tomli to read the lock file so that tomlkit cannot know what type of line endings are in the lock file. If we want to keep existing line endings we either have to read with tomlkit (what this PR is doing) or determine the type of line ending by ourself and somehow set it in tomlkit. I think I would prefer reading with tomlkit (as it is done in this PR) if it was not a performance issue.

#6562 suggests that there is a relevant performance impact:

Even for a modestly sized project like poetry itself, reading poetry.lock on my laptop takes 0.6 - 0.7 seconds (timings obtained with some time.time() statements inserted into the codebase). tomli manages it in ~0.02 seconds.

Thus, maybe we should really check the timings and investigate the alternative approach.

I agree, we can patch tomlkit, but it means tomlkit need to get EOL even using tomlfile.write, which is a bad idea.

this only impact perf when it write lockfile, which mean only add and lock. other command like install, update won't be affected. normally add/lock will spend more time on resolving.

[radoering]

this only impact perf when it write lockfile, which mean only add and lock. other command like install, update won't be affected. normally add/lock will spend more time on resolving.

Good point. I think I will do some simple performance measurements later.

We could basically implement part of the logic of https://github.com/python-poetry/tomlkit/pull/201/files by ourselves, just using TOMLDocument.as_string() instead of TOMLFile.write(). However, that might not be worth it depending on what the timing measurements will reveal.

[dimbleby]

IMO, we have two options:

or, abandon the notion that a particular formatting-style for lockfiles is important and write them with tomli-w - which always uses \n exclusively and therefore can never flip-flop.

I realise that I've suggested this in the past and I guess we likely still disagree on the merits of this - but I continue to favour eliminating tomlkit wherever it is possible to do so.

[dimbleby]

to make the tomli-w comparison concrete I opened https://github.com/python-poetry/poetry/pull/9637

[trim21]

im OK with forcing lf, I just want eol don't change from lf to crlf or crlf to lf.

[github-actions[bot]]

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.