SEPURI-SAI-KRISHNA commented on PR #42411:
URL: https://github.com/apache/superset/pull/42411#issuecomment-5113336062
Pushed a follow-up addressing the bot findings. All five inline threads
point at
code that this revision has deleted or rewritten, so answering them here in
one
place rather than in each outdated thread.
**codeant-ai — English-literal assertion in `schema_tests.py` (flagged
Major)**
Taken. Asserting on the interpolated value is more direct than asserting on
the
wording around it, and it's locale-proof for free — the value comes from
`json.JSONDecodeError`, which the `json` module produces untranslated:
```python
assert "line 1 column" in message
```
The sibling `metadata_params` test already worked that way, asserting on the
offending key rather than the surrounding sentence, so the two are now
symmetric.
I'd push back on "Major"/"possible bug" though. The suite runs under
`BABEL_DEFAULT_LOCALE = "en"`, and eight assertions already in this same
file do
exactly what was flagged:
```python
assert "non-negative integer" in str(exc_info.value) # x5
assert "metadata_cache_timeout must be a mapping" in ... # x2
assert "File contains both" in ...
```
If it were a real defect it would be a pre-existing one affecting those eight
equally, not something this PR introduces. Worth taking as a robustness nit;
not
a bug.
Regression coverage is unchanged — reintroducing `%{msg}s` on the live path
still
fails the test with `ValueError: unsupported format character '{'`.
**Copilot — `messages[0]` will raise because of `field_name`**
False positive, as @rusackas noted. `field_name` doesn't restructure
`messages`;
it only appears in `normalized_messages()`:
```python
>>> ValidationError(['a','b'], field_name='extra').messages
['a', 'b']
>>> ValidationError(['a','b'], field_name='extra').normalized_messages()
{'extra': ['a', 'b']}
```
So `messages[0]` returned the string as intended, which is why those tests
passed
in CI. The module is deleted in this revision regardless.
**Copilot — `assert "%" not in message` is too broad**
Fair, and adopted. The replacement tests assert `"%(" not in message` rather
than
banning `%` outright, exactly as suggested.
--
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]
