search

augustocdias / vscode-shell-command

A task helper to use system commands as input
MIT License
53 stars 20 forks source link

Add an option to error out when the command returns empty string #96

Closed dhlin closed 2 months ago

dhlin commented 2 months ago

Before the fix, if empty result is retuned, the error message is 

Cannot read properties of undefined (reading 'value')

With the fix, the UI can pop up better message for debugging. 

The command for input 'test' returned empty result.
MarcelRobitaille commented 2 months ago

If this is set to false, do you still get the error?

Cannot read properties of undefined (reading 'value')
augustocdias commented 2 months ago

If this is set to false, do you still get the error?

Cannot read properties of undefined (reading 'value')

I was about to ask this... If the intention is to have a nicer message, maybe it is worth to just give the message. I can't see any usecase where empty makes sense... Perhaps ignoring the result and passing an empty string forward could be one, but in that case you'd have to implement this in the PR.

dhlin commented 2 months ago

If this is set to false, do you still get the error?

Cannot read properties of undefined (reading 'value')

If we do "disallowEmptyResult": "false", we don't get the error. While if we do "disallowEmptyResult": false, we get the expected error. There seems to be a general issue for such boolean args being passed as a string, I opened https://github.com/augustocdias/vscode-shell-command/pull/97 to address it. I'll adapt the change once it's merged.

dhlin commented 2 months ago

If this is set to false, do you still get the error?

Cannot read properties of undefined (reading 'value')

I was about to ask this... If the intention is to have a nicer message, maybe it is worth to just give the message. I can't see any usecase where empty makes sense... Perhaps ignoring the result and passing an empty string forward could be one, but in that case you'd have to implement this in the PR.

the way I provide the flag disallowEmptyResult is to avoid any backward compatible issue for current users and to make non invasive changes.

Perhaps ignoring the result and passing an empty string forward could be one, but in that case you'd have to implement this in the PR.

If I understand it correctly, this is to fix the use of nonEmptyInput[0].value; in case nonEmptyInput[0] is undefined ?

MarcelRobitaille commented 2 months ago

If it's only changing an error message to something more useful, I don't see it as a breaking change.

dhlin commented 2 months ago

If it's only changing an error message to something more useful, I don't see it as a breaking change.

Make sense. Changed it to show the new error message directly. It does not depend on https://github.com/augustocdias/vscode-shell-command/pull/97 now

MarcelRobitaille commented 1 month ago

@dhlin I thinks this might have been completed without thinking about every consequence. There have already been 2 issues caused by it (#112 and #107). I think in some cases empty results are allowed. In which situation did you get this:

Cannot read properties of undefined (reading 'value')