Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow use of stderr inside the notifier message #7

Open
0x04 opened this issue Mar 12, 2018 · 15 comments
Open

Allow use of stderr inside the notifier message #7

0x04 opened this issue Mar 12, 2018 · 15 comments
Assignees
Labels
enhancement New feature or request

Comments

@0x04
Copy link
Contributor

0x04 commented Mar 12, 2018

It would be useful if there where a possibility to refer to the stderr in the notifier message. It could be archived with some replacement variable like ${error} or something like that.

@mischah mischah added the enhancement New feature or request label Mar 12, 2018
@mischah
Copy link
Member

mischah commented Mar 12, 2018

I like the idea.

Two questions:

I’m not sure if we should truncate the stderr to make it fit into the notification message 🤔
Does this makes sense? Because the available chars depends on the chars used for the message.

Should we use an easier to type placeholder? e.g. $error

@mischah
Copy link
Member

mischah commented Mar 13, 2018

Hej @0x04, would you like to work on this?

@0x04
Copy link
Contributor Author

0x04 commented Mar 13, 2018

Well, that are good questions.

I'm thinking it makes no sense to truncate the stderr, because the length of message depends on the current OS.

My initial thought was that it would be nice to use the syntax of the template literal, but without getting interpreted it makes less sense. I think we should avoid the use of eval or new Function 😂

@mischah Yeah, why not. Perhaps it is the best to start to experimenting with it, to see which procedure fits best.

0x04 added a commit to 0x04/cli-error-notifier that referenced this issue Mar 29, 2018
@0x04
Copy link
Contributor Author

0x04 commented Mar 29, 2018

After a lot of experimenting, I'm now thinking to do some evaluation on the error literal make sense. If we only put the stderr string in the message/title, this would pretty useless.

Im using new Function to create a independent context for executing the error literal content. This attempt allows to use the error object in the message and title string.

> # Just spit out the whole error message (calls the `toString` method)
> onerror "npm -e test" -t "Error!" -m "Message: {$error}"

> # Access properties of the error object
> onerror "npm -e test" -t "Error! Code ${error.code}" -m "Message: ${error.stderr.substr(0,10)}..."

> # Using a regexp to get a portion of the `stderr` (probably fails)
> onerror "npm -e test" -m "Message: ${error.stderr.match(/[\w\s]+:[\w\s]+\n/)[0]}

> # Making a regexp replace
> onerror "npm -e test" -m "Message: ${error.stderr.replace(/something/, function(m) { return 'somewhat'; })}

The implementation is checking against the error string, to avoid possible misuse. If you wonder why I extract the content of the error literal manually (errorLiteralPosition), it's cause the possibility of nested braces (inline functions etc.) and this is a hard job to do with RegExp. It's very basic and easy to break, but I don't want to include a whole JS parser to get this job done 😄

I'm just drop this lines, because im not sure if you're happy with this. Perhaps you have some comments or ideas on it. If we would use this approach, the template literal syntax would make sense, again 😏

@0x04
Copy link
Contributor Author

0x04 commented Apr 3, 2018

@mischah Sorry, I forgot to ping you 😂

@mischah
Copy link
Member

mischah commented Apr 3, 2018

@0x04 No problem.

Regarding your proposal: Wow 😳
This reads like a super advanced feature.

But to be honest. I’m scared that it’ll blow up the tiny little code base of that module. I’m not quite sure if the usefulness of the feature will compensate the growth of complexity of the codebase.

But I guess we will see 😊

@0x04
Copy link
Contributor Author

0x04 commented Apr 3, 2018

Yeah, the most heavy part of this is not the evalish interpretation of the literal itself. We have a very controlled environment. It's to detect to the portion itself. We could have strings, regexps, objects and functions with a opening/closing brace inside (and all other that I miss). While it's handable in general, it will blew up the code. I just did the most basic thing to play with it.

Well, it works - seems to be a useful tool, but I totally agree with the complexity that comes in. I think it could come very useful if you're using some kinda of IDE like Storm.

I will play around with it's possibilities, perhaps there's something that's less complex.

I will you inform on further progress @mischah!

@0x04
Copy link
Contributor Author

0x04 commented Sep 5, 2019

Okay, I don't think this will make sense in this way of complexity. Closed for now, if I have a enlightment we can reopen it.

@0x04 0x04 closed this as completed Sep 5, 2019
@mischah
Copy link
Member

mischah commented Sep 5, 2019

Thanks Oliver 💖

0x04 added a commit to 0x04/cli-error-notifier that referenced this issue Sep 10, 2019
@0x04
Copy link
Contributor Author

0x04 commented Sep 10, 2019

@mischah Good news, everyone!

I think I had some kind of enlightment! I just thought way to complicated.

Instead of parsing the message string to get the processing code for the ${error} variable, we could pass the this code through a new parameter. This is way easier than try to extract the JavaScript code portion from the message string.

# Old attempt
$ onerror "node -e test" -message "Error: \${error.toString()}"

# New attempt
$ onerror "node -e test" -message "Error: \${error}" -error "error.toString()"

The pseudo signature of the generated function for the error variable subsitution looks as follow:

function (error: object): string {
    return eval(opts.error);
}

The argument error is a childProcessResult Error. So we have access to all child process informations available. The default value for the parameter opts.error is "error.toString()" which just returns the command string.

Further that would allow some cool tweeks to the content of the ${error} variable.

$ onerror "node -e test" -message "\${error}" -error "`Code: \${error.code}`, Command: \${error.cmd.replace(/\r?\n/g, '⏎').substr(0, 15) + '...'}`"

Well, perhaps the processing code gets to long and hard to read - so we could use the power of bash to pass the contents of a JavaScript file:

$ onerror "node - test" -message "\${error}" -error "$(< errorMsg.js)"

Content of errorMsg.js e.g.:

(function(error) {
    const code = error.code;
    const cmd = error.cmd
        .replace(/\r?\n/g, '⏎')
        .substr(0, 15) + '...';

    return `Code: ${code}, Command: ${cmd}`;
})(error);

For Windows-Systems this is a bit harder. cmd isn't able to handle line breaks in variables - but you can assign the contents of a file to a variable. With PowerShell it's more bash-like.

cmd

rem cmd.exe
rem Assign file content to a variable and pass it to `onerror -e`
set /p error=<errorMsg_nolbr.js & onerror "node -e test" -m "${error}" -e "%error%"

PowerShell

# powershell.exe
onerror "node -e test" -m "`${error}" -e "$(type errorMsg.js)"

You can see my first implementation try on my latest commit. Would love to hear your thinking about it.

For now there's no error handling if the evaluation of the processing code fails. However I think it shouldn't be that hard to spit out some useful message to the cli.

Sorry for the shitload of text 😄

@0x04 0x04 reopened this Sep 10, 2019
@mischa
Copy link

mischa commented Sep 10, 2019 via email

@0x04
Copy link
Contributor Author

0x04 commented Sep 10, 2019

Yeah, sorry for that. I forgot a h in the end of mischah's nickname.

@mischah
Copy link
Member

mischah commented Sep 13, 2019

Hej @0x04,

it still sound complicated to me 😅

You can see my first implementation try on my latest commit.

Where? Maybe it would make sense to open a »WIP« PR?

Thanks a bunch 💖

@0x04
Copy link
Contributor Author

0x04 commented Sep 13, 2019

Hi @mischah,

created a »WIP« PR as suggested. I hope I did it right 🤞

@mischah
Copy link
Member

mischah commented Sep 17, 2019

Thanks. Hope to find some time to review this pretty soonish 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants