Why do we need this improvement?
While going through the config context commands, I noticed that all six of them (add, list, edit, remove, use, current) catch errors like this:
if (e instanceof (MissingContextFileError || ContextFileWrongFormatError)) {This only checks MissingContextFileError. The part in brackets runs first, and since a class is always truthy, it just returns MissingContextFileError. So ContextFileWrongFormatError is never actually checked here.
Right now the behaviour is correct, but only by accident. When the context file is broken, the error skips this branch and the user sees the real message ("Context file ... has wrong format"). The existing tests for add expect exactly that message.
The problem is that the code says something different from what it does. If someone tries to "fix" it to check both errors, users with a broken context file would be told they have no context file and should run asyncapi config context init. But init writes a new file without checking if one already exists, so they would lose their saved contexts. That change also makes 4 of the existing add tests fail, so it's an easy mistake to make.
There is also a small dead check in current.ts. (fileContent && !fileContent.current) is inside the catch block, but fileContent is always undefined there, so it can never be true.
How will this change help?
The code will say what it actually does, so it's clear that a broken context file should show the "wrong format" error and not the "run init" message. It also stops someone from bringing in the bug above during a future cleanup.
Right now only add has tests for a broken context file. Adding the same tests for the other five commands means this behaviour is covered everywhere.
Screenshots
No response
How could it be implemented/designed?
It's a small change:
- In all six commands, replace
e instanceof (MissingContextFileError || ContextFileWrongFormatError) with e instanceof MissingContextFileError
- Remove the
ContextFileWrongFormatError imports that become unused
- Remove the dead
(fileContent && !fileContent.current) check in current.ts
- Add tests in
test/integration/context.test.ts that run current, list, use, edit and remove with a broken context file and check that the "has wrong format" message is shown
Nothing changes for users. Happy to open a PR for this if it sounds good.
🚧 Breaking changes
No
👀 Have you checked for similar open issues?
🏢 Have you read the Contributing Guidelines?
Are you willing to work on this issue?
Yes I am willing to submit a PR!
Why do we need this improvement?
While going through the
config contextcommands, I noticed that all six of them (add,list,edit,remove,use,current) catch errors like this:This only checks
MissingContextFileError. The part in brackets runs first, and since a class is always truthy, it just returnsMissingContextFileError. SoContextFileWrongFormatErroris never actually checked here.Right now the behaviour is correct, but only by accident. When the context file is broken, the error skips this branch and the user sees the real message ("Context file ... has wrong format"). The existing tests for
addexpect exactly that message.The problem is that the code says something different from what it does. If someone tries to "fix" it to check both errors, users with a broken context file would be told they have no context file and should run
asyncapi config context init. Butinitwrites a new file without checking if one already exists, so they would lose their saved contexts. That change also makes 4 of the existingaddtests fail, so it's an easy mistake to make.There is also a small dead check in
current.ts.(fileContent && !fileContent.current)is inside thecatchblock, butfileContentis alwaysundefinedthere, so it can never be true.How will this change help?
The code will say what it actually does, so it's clear that a broken context file should show the "wrong format" error and not the "run init" message. It also stops someone from bringing in the bug above during a future cleanup.
Right now only
addhas tests for a broken context file. Adding the same tests for the other five commands means this behaviour is covered everywhere.Screenshots
No response
How could it be implemented/designed?
It's a small change:
e instanceof (MissingContextFileError || ContextFileWrongFormatError)withe instanceof MissingContextFileErrorContextFileWrongFormatErrorimports that become unused(fileContent && !fileContent.current)check incurrent.tstest/integration/context.test.tsthat runcurrent,list,use,editandremovewith a broken context file and check that the "has wrong format" message is shownNothing changes for users. Happy to open a PR for this if it sounds good.
🚧 Breaking changes
No
👀 Have you checked for similar open issues?
🏢 Have you read the Contributing Guidelines?
Are you willing to work on this issue?
Yes I am willing to submit a PR!