SEPURI-SAI-KRISHNA commented on PR #42411:
URL: https://github.com/apache/superset/pull/42411#issuecomment-5113129894
Thanks @rusackas — you're not missing a caller, it's dead code. I dug in and
it's
worse than my original scope note suggested, so I've pushed a commit
pivoting the
PR to delete both classes rather than fix a copy nobody reaches.
You said it didn't block either way, so to be clear: **this is easy to
undo.** If
you'd rather keep the classes and take just the original one-character
`%{key}s` → `%(key)s` fix, say the word and I'll drop the deletion commit —
the
new `extra_validator` tests stand on their own either way. I went ahead
because
the evidence was conclusive, but it's your call, not mine.
What I found:
- Both classes came in with #10741 (2020-09-02). Grepping that commit's diff
shows only the `class` statements were added — no `raise` site ever came
with
them.
- `git log -S "DatabaseExtraValidationError"` returns exactly that one
commit.
- Of the 37 classes in `superset/commands/database/exceptions.py`, these two
are
the only ones with no reference anywhere else in the tree.
- And as you say, `extra_validator` in `superset/databases/schemas.py`
already
raises both messages inline with the correct `%(key)s` / `%(msg)s`. The
dead
copy was a stale duplicate that had drifted into the invalid placeholder.
So the PR now removes both classes. I also noticed `extra_validator` has ~13
tests but none covering either of those two messages, so I added two next to
the
existing ones in `schema_tests.py` that render each message and assert it
interpolated. Reverting `%(key)s` to `%{key}s` on the live path now fails
with
the same `ValueError: unsupported format character '{'` the deleted class
produced — so the typo can't come back where users would actually hit it.
Agreed on Copilot's threads being false positives —
`ValidationError.messages`
stays the list you passed it and `field_name` only shows up in
`normalized_messages()`. Its narrower nit about `assert "%" not in message`
being
over-broad was fair though, so the new assertions check for `%(`
specifically.
The test file it commented on is gone either way. Branch name still says
`placeholder` — renaming would close the PR, so I left it.
Happy to add an `UPDATING.md` entry if you think removing importable symbols
warrants one; I left it out since that file seems to track operator-facing
changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
