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

Display cvs.Error without TypeError context #114628

Closed
smontanaro opened this issue Jan 26, 2024 · 9 comments
Closed

Display cvs.Error without TypeError context #114628

smontanaro opened this issue Jan 26, 2024 · 9 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@smontanaro
Copy link
Contributor

smontanaro commented Jan 26, 2024

Bug report

Bug description:

Working my way through some documentation nits for the csv module, I noticed this method definition:

    def _validate(self):
        try:
            _Dialect(self)
        except TypeError as e:
            # We do this for compatibility with py2.3
            raise Error(str(e))

Why should we still try and maintain compatibility with Python 2.3 at this late date? Shouldn't we just let TypeError bubble up to the user?

CPython versions tested on:

3.13

Operating systems tested on:

macOS

Linked PRs

@smontanaro smontanaro added the type-bug An unexpected behavior, bug, or error label Jan 26, 2024
@gaogaotiantian
Copy link
Member

I guess backward compatibility is a contagious thing. The comment might be out of date, but this is effectively the behavior from the ancient time until now. Changing that will not only break backward compatibility for Python 2.4, but also all the versions till 3.12.

@ericvsmith
Copy link
Member

Right. I believe that comment can be read to say "Since this behavior goes back to at least Python 2.3, it can't be changed with a deprecation cycle". Maybe the comment should be changed to say that.

@terryjreedy
Copy link
Member

csv.Error is a subclass of Exception. It is documented as being raised in 6 different methods. (Have not check if all true.) Many modules define their own exception or exceptions to raise in module methods. So csv is not exceptional in this, though it is inconsistent that ValueError is raised in a couple of places. The comment is obsolete and should be deleted. The other place Error is raised in based on 'if' instead of 'for' so the message is constructed directly instead of via an stdlib exception.

CSVError might have been a better name, and could even be added as an alias. But I would push for that here.

@ericvsmith
Copy link
Member

I agree we should just delete the comment.

CSVError might have been a better name, and could even be added as an alias. But I would push for that here.

Do you mean "would not"? Otherwise I don't understand the "But" part of that sentence. I'd be against an alias, although I agree CSVError would have been better.

@smontanaro
Copy link
Contributor Author

One other thing occurred to me. I'll accept that raising csv.Error is fine given the history and the long deprecation process required to change things. Could it not be raised while preserving the stack frames from the original TypeError though?

@terryjreedy
Copy link
Member

would not

@terryjreedy
Copy link
Member

terryjreedy commented Feb 4, 2024

Skip, in current Python, an exception raised within except by default gets the outer exception attached as .__context__ and both are displayed if the inner exception is not caught.

>>> csv.excel.delimiter = 1
>>> csv.excel()
Traceback (most recent call last):
  File "C:\Programs\Python313\Lib\csv.py", line 52, in _validate
    _Dialect(self)
TypeError: "delimiter" must be string, not int

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<pyshell#8>", line 1, in <module>
    csv.excel()
  File "C:\Programs\Python313\Lib\csv.py", line 48, in __init__
    self._validate()
  File "C:\Programs\Python313\Lib\csv.py", line 55, in _validate
    raise Error(str(e))
_csv.Error: "delimiter" must be string, not int

If one raises the inner exception from outer (see "The from clause"), the outer become the .__cause__ attribute. This changes the linking sentence ('During' above). If both tracebacks were useful, that would likely be preferable. However, in the above, the TypeError traceback is redundant and should be suppressed with from None. It is defective in that it does not show which Dialect subclass has the bad attribute. The csv.Error traceback does, which justifies re-raising.

I will make a PR revising the comment and adding 'from None'. The only other occurrence of raise Error in the file is in an if statement.

terryjreedy added a commit to terryjreedy/cpython that referenced this issue Feb 4, 2024
When cvs.Error is raised when TypeError is caught,
the TypeError display and 'During handling' note is just noise
with duplicate information.  Suppress with 'from None'.
@terryjreedy terryjreedy changed the title Probably shouldn't continue with Python 2.3 compatibility in csv.py Display cvs.Error without TypeError context Feb 4, 2024
@terryjreedy terryjreedy added type-feature A feature request or enhancement stdlib Python modules in the Lib dir and removed type-bug An unexpected behavior, bug, or error labels Feb 5, 2024
terryjreedy added a commit that referenced this issue Feb 5, 2024
When cvs.Error is raised when TypeError is caught,
the TypeError display and 'During handling' note is just noise
with duplicate information.  Suppress with 'from None'.
@terryjreedy
Copy link
Member

While from None could have been added here anytime after being added in to Python in 3.3, it is not a bug that it had not been before. Hence no backport.

@smontanaro You are listed as inactive csv expert. You seem to be active again. Change back to active (delete (inactive))? Are you aware/involved with CSV Issues gh project? If so, OK if I add new field Type: General, Dialect, or Sniffer? to categorize issues?

@smontanaro
Copy link
Contributor Author

@smontanaro You are listed as inactive csv expert. You seem to be active again. Change back to active (delete (inactive))? Are you aware/involved with CSV Issues gh project? If so, OK if I add new field Type: General, Dialect, or Sniffer? to categorize issues?

I'm really not interested in being "the CSV guy" (never really was, was just the last person involved). I retired several years ago and now mostly just noodle around with Python. I was more-or-less house-bound the past week or so, and stumbled on #101100, so spent some time there. I'll probably herd the few open tickets along, then get back to the other things I generally do with my time.

As for the CSV module, I could solve several of the items on that list by simply deleting the csv.Sniffer class, but I suppose we're stuck with it. I never liked or needed it.

aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
When cvs.Error is raised when TypeError is caught,
the TypeError display and 'During handling' note is just noise
with duplicate information.  Suppress with 'from None'.
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this issue Feb 14, 2024
When cvs.Error is raised when TypeError is caught,
the TypeError display and 'During handling' note is just noise
with duplicate information.  Suppress with 'from None'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
Status: Done
Development

No branches or pull requests

4 participants