Fix: indicate location of toml file in question when raising a TOMLDecodeError#10647
Fix: indicate location of toml file in question when raising a TOMLDecodeError#10647JaviMaligno wants to merge 3 commits intopython-poetry:mainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideWraps reading of the local TOML configuration in a try/except so that TOML parsing errors are re-raised as PoetryRuntimeError with the path of the problematic config file included in the message. Sequence diagram for error handling when loading local TOML configurationsequenceDiagram
actor Developer
participant CLI as PoetryCLI
participant Factory as Factory
participant Config as Config
participant LocalFile as LocalConfigFile
participant TOML as TOMLParser
Developer->>CLI: run_command()
CLI->>Factory: create_poetry(io)
Factory->>LocalFile: path
Factory->>LocalFile: read()
LocalFile->>TOML: parse_toml(content)
alt TOML parsing succeeds
TOML-->>LocalFile: parsed_data
LocalFile-->>Factory: parsed_data
Factory->>Config: merge(parsed_data)
Config-->>Factory: merged_config
Factory-->>CLI: poetry_instance
else TOML parsing fails
TOML-->>LocalFile: TOMLError
LocalFile-->>Factory: TOMLError
Factory->>Factory: catch TOMLError
Factory-->>CLI: raise PoetryRuntimeError("Invalid TOML configuration file <path>: <TOMLError>")
CLI-->>Developer: display error with file path
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The PR title and issue reference mention
TOMLDecodeError, but the code catchesTOMLError; consider aligning the exception type (or explicitly documenting why the broader exception is needed) to avoid confusion. - Raising
PoetryRuntimeErrorwhere aTOMLErrorwas previously propagated may change error-handling behavior for callers; consider whether the original exception type should be preserved or wrapped in a way that keeps compatibility with existing exception handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PR title and issue reference mention `TOMLDecodeError`, but the code catches `TOMLError`; consider aligning the exception type (or explicitly documenting why the broader exception is needed) to avoid confusion.
- Raising `PoetryRuntimeError` where a `TOMLError` was previously propagated may change error-handling behavior for callers; consider whether the original exception type should be preserved or wrapped in a way that keeps compatibility with existing exception handling.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
The title says something about the location of the toml file but the code does not any such information if I do not miss anything. I do not think this fixes #10270. |
|
Thanks for the feedback @radoering! You're right that the error message was missing the file location. I've updated the code to include except TOMLError as e:
raise PoetryRuntimeError(
f"Problem processing TOML configuration in {self.project_directory}: {e}"
) from eNow when users encounter a TOML parsing error, they'll see which project directory has the issue, making it easier to identify the problematic file. |
|
I think this still does not solve the issue described in #10270:
However, there already has been such a fix in python-poetry/poetry-core#734. This fix is included in Poetry 2.0. The issue was reported with Poetry 1.7.1. Actually, I think the issue has already been fixed and your change does not add any additional information. I closed the issue and will close this PR. Thank you anyway for the effort. |
|
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. |
Fixes #10270
Summary by Sourcery
Bug Fixes: