Skip to content

Fix unformatted placeholders in secondary_y and output_type error messages - #5772

Merged
camdecoster merged 3 commits into
plotly:mainfrom
David-Wu1119:unformatted-error-placeholders
Oct 22, 2026
Merged

camdecoster merged 3 commits into
plotly:mainfrom
David-Wu1119:unformatted-error-placeholders

Conversation

@David-Wu1119

@David-Wu1119 David-Wu1119 commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Link to issue

No existing issue: a search for both messages found none.

Description of change

Two ValueError messages were never formatted, so users see the raw placeholders:

fig = make_subplots(rows=1, cols=2)
fig.add_scatter(x=[1], y=[1], row=1, col=2, secondary_y=True)
# ValueError:
# Subplot with type '{subplot_type}' at grid position ({row}, {col}) was not
# created with the secondary_y spec property set to True. ...

pio.from_json(fig.to_json(), output_type="Bogus")
# ValueError:
# Invalid output type: {output_type}

This PR makes both messages f-strings. After the change they read Subplot with type 'xy' at grid position (1, 2) was not ... and Invalid output type: Bogus.

Testing strategy

No new tests or changelog entry, per review. Checked by hand that both messages now read Subplot with type 'xy' at grid position (1, 2) was not ... and Invalid output type: Bogus. tests/test_core/test_subplots/ and tests/test_io/test_to_from_json.py pass (106 tests), and ruff format --check and ruff check with the pinned 0.11.12 pass on the changed files.

Guidelines

Found with a scan for string literals that contain {name} placeholders but are never formatted; written with AI assistance.

🤖 Generated with Claude Code

The error raised when adding a trace with `secondary_y=True` to a subplot
that was not created with the `secondary_y` spec, and the error raised for
an invalid `output_type` in `plotly.io.from_json`/`read_json`, were plain
strings that were never formatted, so users saw the literal
"{subplot_type}", "({row}, {col})" and "{output_type}". Format them like
the neighbouring messages in the same functions already do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 23, 2026 17:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@camdecoster camdecoster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! Could you please convert these to f-strings? Also, please remove the tests you added. They aren't needed. We also won't update the changelog for a change like this, so could you please revert that change?

@camdecoster camdecoster self-assigned this Sep 23, 2026
Per review: format the two messages with f-strings instead of
str.format(), and remove the added tests and CHANGELOG entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@David-Wu1119

David-Wu1119 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks @camdecoster! Done in dd7cdd4: switched both messages to f-strings and removed the tests and the changelog entry, so the diff is now just the two lines in plotly/_subplots.py and plotly/io/_utils.py.

@camdecoster camdecoster added the no-changelog A changelog entry is not required for this PR label Oct 22, 2026

@camdecoster camdecoster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this!

@camdecoster
camdecoster merged commit 63895f5 into plotly:main Oct 22, 2026
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog A changelog entry is not required for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants