Add attributes to TOMLDecodeError

[hukkin]

Adds the same attributes to TOMLDecodeError that JSONDecodeError has.

Created a discussion about this https://discuss.python.org/t/69546

edit: and a cpython issue python/cpython#126175

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (59ed9ef) to head (55ad7ab).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #238   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          510       527   +17     
  Branches        93        97    +4     
=========================================
+ Hits           510       527   +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[encukou]

JSONDecodeError accepts positional arguments, maybe TOMLDecodeError should too to be more compatible?
Perhaps something like:

class TOMLDecodeError(ValueError):
    """An error raised if a document is not valid TOML."""
    def __init__(self, msg: str = None, doc: str = None, pos: Pos = None, *args):
        if (
            args
            or not isinstance(msg, str)
            or isinstance(doc, str)
            or not isinstance(pos, Pos)
        ):
            warnings.warn(
                "Free-form arguments for TOMLDecodeError are deprecated. "
                "Please set 'msg', 'doc' and 'pos' arguments.",
                DeprecationWarning,
                stacklevel=2,
            )
            if pos is not None:
                args = pos, *args
            if doc is not None:
                args = doc, *args
            if msg is not None:
                args = msg, *args
            super().__init__(*args)
        else:
            lineno = doc.count("\n", 0, pos) + 1
            if lineno == 1:
                colno = pos + 1
            else:
                colno = pos - doc.rindex("\n", 0, pos)

            if pos >= len(doc):
                coord_repr = "end of document"
            else:
                coord_repr = f"line {lineno}, column {colno}"
            errmsg = f"{msg} (at {coord_repr})"
            super().__init__(self, errmsg)

            self.msg = msg
            self.doc = doc
            self.pos = pos
            self.lineno = lineno
            self.colno = colno

(Not sure if typechecker will allow str & Pos rather than str|None & Pos|None, but it would be nice to show the non-deprecated signature! For stdlib, typeshed can IMO list the new signature only.)

[hukkin]

I think that would break exc.args in cases where:

• 3 positional args of types (str, str, int) are given
• any of the first 3 positional args passed in is None

Notably, the changing of exc.args in these cases also changes the output of str(exc) and repr(exc).

If this is acceptable, let's do it.

An alternative is what's currently in this PR, and allowing JSONDecodeErrors signature (positional args included) after the deprecation period, which I think shouldn't ever break anyone, but has the downside that it doesn't immediately allow the final signature.

Not sure if typechecker will allow str & Pos rather than str|None & Pos|None, but it would be nice to show the non-deprecated signature

I think this should be no issue at all. We can type annotate the non-deprecated signature, and # type: ignore the lines in TOMLDecodeError.__init__ that violate this, and that should be it.

typeshed can IMO list the new signature only

I agree.

[nineteendo]

• any of the first 3 positional args passed in is None

You could use a private sentinel, but this is overkill, the backwards compatibility policy isn't that strict.

[hukkin]

I read PEP 387 and my interpretation is it doesn't seem to be lenient in a way that's applicable here.

Unless it is going through the deprecation process below, the behavior of an API must not change in an incompatible fashion between any two consecutive releases.

If a change is breaking, it seems very strict, no exceptions. Where it does seem lenient is interpreting whether or not a change is breaking:

This PEP takes the perspective that “backwards incompatibility” means preexisting code ceases to comparatively function after a change. It is acknowledged that this is not a concrete definition, but the expectation is people in general understand what is meant by “backwards incompatibility”

If someone shows up and says we broke their TOMLDecodeError("str", "str", 0) instantiation, I don't think an argument can be made here that our change was back compatible.

I understand if in practice the PEP is applied pragmatically, and it's ok to break use cases that seem extremely rare or low importance, etc, but to me it seems like the PEP doesn't say it's allowed. Should it?

[encukou]

Yup, there's a judgment call.

The chances of someone using ("str", "str", 0) or ("str", None), and using args to get the info out, seem vanishingly low. (And using a private sentinel rather than None would push that even lower.)

The original version of PR would break code that's not common, but it's likely to exist -- using one positional string (code copied from tomllib itself), or no arguments (lowest-effort way to mock the exception). Those should definitely keep working -- along with ones where there's no other reason to change them.
Here, we're trying to match JSONDecodeError, which is overwhelmingly called with positional arguments. Changing the (msg, doc, pos) case is, IMO, worth betting that no one uses this to fill in args.

[hukkin]

Thanks a lot for the reviews and comments!

Yeah, I agree this makes sense. I was mainly wondering whether making such judgment calls should be explicitly allowed by the PEP, as my interpretation was they may currently not be.

I've pushed another commit that includes a couple tests. As far as I'm aware, this revision doesn't break anything except the highly unlikely TOMLDecodeError("str", "str", 0).args attribute access (and consequently str(e) in the same case).

I'll make a sibling PR to cpython in the upcoming days.

[encukou]

I was mainly wondering whether making such judgment calls should be explicitly allowed by the PEP, as my interpretation was they may currently not be.

Well, here I am essentially betting that no one, so far, wrote code that would be broken by this -- i.e. the “preexisting code” doesn't exist.
If I'm wrong, we'll need to revert.