Avoid deprecation warning from Tomli

[hukkin]

Universal newlines are not TOML compatible.

See e.g. toml-lang/toml#837 for context. EDIT: this is no longer relevant to this PR given the new title and current changes.

[uranusjr]

So if I understand this correctly, this only affect string literals? If that’s the case, this can probably not too urgent and can wait until 21.3 (since long string literals are not at all common in pyproject.toml right now). Otherwise we should backport this to 21.2.

[hukkin]

This mostly just makes the parser throw an exception if CR characters are used as line endings, anywhere in the document, as that is invalid TOML. I don't think this is in any way urgent at all but is more technically correct, 😄 FWIW.

[uranusjr]

Oh, so I’m thinking bckwards; the current implementation silently coerce CR (due to universal newlines), which is technically incorrect? If that’s the case, maybe we should keep doing that since banning CR all the sudden can be too disruptive to Windows users.

[hukkin]

Yeah current implementation converts CRs (and CRLFs) to LFs. The correct behavior is to error on bare CR, because it is not an allowed line ending in TOML.

If that’s the case, maybe we should keep doing that

That should also work. Note that I have deprecated text file objects as tomli.load input (in favor of binary file objects) because TOML spec is not nearly as liberal about encoding and newlines as Python text file objects. For 100% compliancy TOML requires UTF-8 and disabling universal newlines, which are incredibly easy to forget to configure. Thus, if you want to avoid a deprecation warning in tomli v1.2.0+ (while still allowing spec incompatible CR line endings) you should move from

with open(pyproject_toml, encoding="utf-8") as f:
    pp_toml = tomli.load(f)

to something like

with open(pyproject_toml, encoding="utf-8") as f:
    pp_toml = tomli.loads(f.read())

since banning CR all the sudden can be too disruptive to Windows users.

Banning CR should not disrupt Windows users (Windows uses CRLF which will still work fine). My understanding is that CR as line ending is used by very old Mac OSes (pre Mac OS X (released 2001)) and other exotic legacy systems.

[pfmoore]

So I'm confused. On Windows, text files (which TOML files need to be considered as, because a huge part of the reason for using TOML as the format for Python packaging is that it's human readable/editable) can use LF or CRLF line endings, as text editors typically produce either. And of course projects developed on Windows being built on Unix could have pyproject.toml with either line ending.

I'd say that pyproject.toml must allow LF and CRLF interchangeably as line endings on all platforms. If that means we're now technically using a variant of TOML, then I think we need to explicitly note that fact in PEP 518.

If the prohibition is just about bare CR, not CRs that are part of a CRLF pair, then the above is irrelevant and you can ignore me (except as a warning to be careful how you present this information, if you want to avoid scaring people!)

[hukkin]

If the prohibition is just about bare CR, not CRs that are part of a CRLF pair, then the above is irrelevant and you can ignore me (except as a warning to be careful how you present this information, if you want to avoid scaring people!)

This is only about bare CR. Sorry, didn't mean to scare anyone 😄

[pradyunsg]

Please avoid merging this without an OK from me -- I haven't had time to take a look at the issue on the TOML repository, but this is definitely not the direction to take IMO.

[hukkin]

Suggested change

	with open(pyproject_toml, "rb") as f:
	pp_toml = tomli.load(f)
	with open(pyproject_toml, encoding="utf-8") as f:
	pp_toml = tomli.loads(f.read())

BTW, if the "rb" change is rejected, I think we should commit this change to keep existing behavior while avoiding deprecation warning from Tomli.

[pradyunsg]

Let's do this.

[hukkin]

👍 Pushed this change.

[pradyunsg]

To communicate with sufficient clarity: I do not intend to block the 21.3 release on this PR, since this doesn't seem to be a show-stopper issue for the release. To that end, I've gone ahead and dropped this from the release milestone. :)

[hukkin]

@pradyunsg this PR in its current state no longer needs to update vendored Tomli so I've reverted that change. Am I right that we can skip the news entry with this one?

Also, I changed the title of this PR to reflect current changes.

[pradyunsg]

Yea, this works! :)