GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-124835: `tomllib.loads`: Raise TypeError not AttributeError. Improve message by hukkin · Pull Request #124587 · python/cpython

Conversation

@hukkin

@hukkin hukkin commented Sep 26, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This PR makes tomllib.loads(b"") throw

TypeError: Expected str object, not 'bool'

instead of

TypeError: a bytes-like object is required, not 'str'

Given an input type that doesn't have the replace attribute, it makes tomllib.loads raise the same new TypeError, instead of an AttributeError. Should this be a separate PR though?

The corresponding Tomli PR is hukkin/tomli#229
The issue report is hukkin/tomli#223
Pinging @encukou as requested here hukkin/tomli#223 (comment)

  • Issue: Improve invalid input type handling in tomllib.loads #124835

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@hukkin
tomllib.loads: Raise TypeError not AttributeError. Improve message
1c16de5
@hukkin
hukkin requested review from encukou and hauntsaninja as code owners September 26, 2024 09:20
@bedevere-app bedevere-app Bot added the awaiting review label Sep 26, 2024
@hukkin
Fix variable name
9d97202
picnixz
picnixz reviewed Sep 26, 2024
View reviewed changes

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two stances:

  • I'm ok with the change but I would let a regular TypeError be propagated instead (e.g., if someone is using a subclass os str for whatever reason).
  • I'm not ok with the change because of the "garbage in, garbage out" principle, though it's not really an in/out here. I think loads should only expect a string.

For the second point, we could do something like isinstance(s, str) but this would restrict the user to always use a string instance and not a UserString object for instance.

I don't know which one is better. But the fact that the message does not help is IMO an inconvenience so I like that you want to fix it. Now, how to fix it, is still something I'm pondering.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tomllib/test_error.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz reviewed Sep 26, 2024
View reviewed changes
Comment thread Lib/tomllib/_parser.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz

picnixz commented Sep 26, 2024

Copy link
Copy Markdown
Member

I suggest opening a CPython issue so that we can possibly discuss it. In addition, I think a NEWS entry should be added so that people know about the fact that an AttributeError won't be raised (but for now I'll just add the skip news label)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz picnixz added the skip news label Sep 26, 2024
@hukkin
Fix variable name pt. 2
23ad2ef
@JelleZijlstra

JelleZijlstra commented Sep 26, 2024

Copy link
Copy Markdown
Member

Yes, this should have a NEWS entry (and therefore, an issue) as it changes user-visible behavior.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz picnixz removed the skip news label Sep 26, 2024
@hukkin hukkin mentioned this pull request Oct 1, 2024
Improve invalid input type handling in tomllib.loads #124835
Closed
@hugovk hugovk changed the title tomllib.loads: Raise TypeError not AttributeError. Improve message gh-124835: tomllib.loads: Raise TypeError not AttributeError. Improve message Oct 1, 2024
@hukkin
Add NEWS
4c85b60
@hukkin

hukkin commented Oct 1, 2024

Copy link
Copy Markdown
Contributor Author

Thanks! I've opened an issue (#124835) and added a NEWS entry.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
hauntsaninja
hauntsaninja approved these changes Oct 2, 2024
View reviewed changes

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Oct 2, 2024
@hauntsaninja
hauntsaninja merged commit 9ce9020 into python:main Oct 2, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 2, 2024
@hukkin
hukkin deleted the tomllib-typeerror branch October 2, 2024 03:49
@AA-Turner AA-Turner mentioned this pull request Jan 30, 2025
Improve importlib backports-upstream integration #129307
Open
@dargueta-accompanyhealth dargueta-accompanyhealth mentioned this pull request Apr 17, 2025
Passing bytes to tomllib.loads() gives wrong error message #132655
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@picnixz picnixz picnixz left review comments
@hauntsaninja hauntsaninja hauntsaninja approved these changes
@encukou encukou Awaiting requested review from encukou encukou is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@hukkin @picnixz @JelleZijlstra @hauntsaninja
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.