-
Notifications
You must be signed in to change notification settings - Fork 61
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
Ability to disable printf formatting #57
Comments
There's an easy way to get the behaviour you're after with the existing API: var mod_verror = require('verror');
var unsanitised_input = '......%.....'; /* Some string with a % in it, e.g. from user input. */
var err = new mod_verror.VError('%s', unsanitised_input); |
I'm aware of using With the rise of template literals, I've seen a lot of codebases use new VError(`${foo} ${bar}`) In other words, users can completely ignore VError's printf behavior, because it doesn't seem necessary, until they later run into the |
We could provide a new top-level constructor for this. |
I threw up #58 to add |
This works fine: new VError('This can have % in it without crashing'.replace('%', '%%')); |
That actually fails in a few ways: new VError('this will still throw %%s'.replace('%', '%%'));
new VError('you forgot to handle multiple placeholders: %s %s'.replace('%', '%%')); I think this illustrates how tricky getting this right can be. All the more reason VError should offer some help. And even if you could sanitize reliably, it would suffer the same problem that |
This is just to escape the message, I can not know when it can be useful and when it is useful just add a few characters to solve the problem. .replace(/%/g, '%%') |
@gabegorelick I assume you do want to add a new top-level constructor -- isn't that the primary goal? I'd suggest calling the option something like |
Correct, but starting small first. I'll try to get something up to Gerrit now. |
https://cr.joyent.us/#/c/4394/ (didn't take as long as I thought 😄). |
(copying from the Gerrit review, just in case) I'm very sorry that this fell off my radar. It's been way too long. I've made a few comments on the changes. Broadly, they still look great. I really appreciate your willingness to put this change together, and I imagine some of this seems pretty nitty -- if you'd like, I'd be happy to make these suggested changes. |
@davepacheco Sorry, this fell off my radar too.
If you have the bandwidth, go for it! |
This was picked up by another change described under #63. |
This just caused my team days of investigation due to an insanely hard to re-create error situation where an error message eventually had We would get timeouts and users would report them but couldn't find out why without a deep dive into the logs (we have millions of requests a hour) and singling out what could possibly have caused some random error with our code. Have bottom line if I send a message and no arguments then it shouldn't use the Indeed when we found out what was happening we did end up doing ( Yes i realize "52% percent" is odd ;-), not my area or concern haha ) tl;drmake printf opt-in not opt-out |
I'd like to open by pointing out in no uncertain terms that this is the kind of oddly misplaced aggression that completely exhausts maintainers. Our library is (I feel) clearly documented to take In addition, as @davepacheco noted above, it seems that the work on #63 is going to include a new option to disable
We're unlikely to make such a breaking change to the existing |
Is there a single case that not providing a second argument would provide the desired effect with sprintf? Should (“%”) not work fine? Doesn’t seem breaking since those using the library would have an error anyway in the case they aren’t providing the argument, unless I’m missing something ofc |
I really encourage you to read #63, where more discussion about this has occurred. There is some mention of other times we've hashed this out already, in particular this question and answer on #8. |
Reminder for myself about the history here: @gabegorelick submitted #58 for this, then moved that to cr.joyent.us as https://cr.joyent.us/#/c/4394/. That seemed to stall after I took an embarrassingly longtime to provide feedback. Then @hekike submitted #63, which is pretty similar. There's a proposed change on cr.joyent.us at https://cr.joyent.us/#/c/5360/. That also seems to have stalled. To avoid future confusion I'm going to close this issue but the request is still open under #63. Moving discussion there. |
Lots of use cases for VError have no need for printf. But VError doesn't seem to have a way to opt out of printf.
This is especially inconvenient given that a string with
%
in it can cause VError to throw an Error, which has been a source of frustration for lots of users.Potential API:
The text was updated successfully, but these errors were encountered: