🐛 Fix display of metavar argument in Rich formatted help output#1408
🐛 Fix display of metavar argument in Rich formatted help output#1408Mohammed-Saajid wants to merge 2 commits intofastapi:masterfrom
metavar argument in Rich formatted help output#1408Conversation
svlandeg
changed the title
metavar argument in Rich formatted help output
svlandeg
changed the title
metavar argument in Rich formatted help outputmetavar argument in Rich formatted help output
svlandeg
left a comment
svlandeg
left a comment
There was a problem hiding this comment.
Hi @Mohammed-Saajid, thanks for the PR! Can you clarify if any parts of this have been vibe-coded, and if so, which parts?
|
Hi @svlandeg, The Core fix of the bug was manually done. But, tests was generated using AI. Even though I have checked the test file, I'll implement any further changes upon request. Kindly let me know. Thank you and Sorry for inconvenience if any. |
svlandeg
left a comment
svlandeg
left a comment
There was a problem hiding this comment.
The underlying issue here in the code base is pretty complex. For a PR to fix it, ideally you should write the (breaking) unit tests first, commit them, and only then implement and commit the fix. Writing the tests afterwards, especially by vibe coding them, is a kind of self-fulfilling prophecy and gives a false sense of security. Additionally, no explanation was given in the PR body for the fix or the tests, which doesn't help maintainers who need to review this.
I suggest to close this one in favour of #1410, which is more extensive and has more detailed tests taken from user discussions around this topic. Though the general direction of the fix in this PR is correct, some of the tests in #1410 will still fail on this PR.
|
Yes, I completely agree. next time I will make sure to meet the quality standards. Thank you. |
2 participants
Fixed #1156