Today I got an error from @sapphire/shapeshift which wasn't very helpful as it didn't say what was wrong. Is it possible to improve these errors?
#Improve errors from shapeshift
inner gyro
teal basin
this is on you & how you log those errors
"improving" this as you put it would mean stripping away information in scenarios where it might be extremely relevant
notice how there's an unexpanded ValidationError on your first element in the array
log with higher depth and see what it is
olive locust
I actually ran into a similar thing when I didn't try/catch the error - Nodes default unhandled rejection logging doesn't use Util.inspect, but console.log/error do. If you catch it, you'll get much better output
Annoying though I know since our guide has builders as top level exports
inner gyro
Thing is this is a crash log, I can't really expand it afterwards and catching these is almost impossible, it's usually better to read in the console after the crash so we can fix it, and this way we can't really fix it
teal basin
well there's not much that can be done re the errors themselves
olive locust
It's definitely not impossible but I otherwise I agree with the frustration
teal basin
the error is incredibly verbose because you can have really complicated schemas and you need a super complete error to know exactly why validation failed
which is why for things like unions (for example) shapeshift will give you an array of errors, telling you why each and every member of the union didn't match
and while that's very hard to read given you were intending to hit one of them
inner gyro
but you do realize that saying "Received one or more errors" is extremely useless when you could just say what errors were received
teal basin
it's.. just.. not dropping information
uh, what?
again, the error(s) you received are there
just under an array
inner gyro
they are there, but the initial information is useless
teal basin
yes, that's a CombinedPropertyError
in your case given there's one error, it can seem useless to have things nested like this
but again, in more complex cases "simplifying" this would lead to stripping relevant information
inner gyro
but if we're not expecting any errors (which, most of the time, we aren't) getting a crash log like this won't help us fix the issue so it would be better to at least say something that was wrong so that we don't need to guess it
teal basin
sure, figure out how to do that and PR it to shapeshift 
I'm sure they'll appreciate it
inner gyro
Well my idea was to have the error message be the first error's message and then leave the others like they already appear, wouldn't that work?
teal basin
this is out of scope for here at this point, probably
faint sorrel
isn't shapeshift developed by the same guys who maintain djs
yes, but its still from @graceful veldt and belongs there
inner gyro
I figured I'd make a PR to make it so that if there's 1 error it shows the error message and if there's multiple is says there's multiple, I'll close this now since it isn't quite djs
glossy sparrow
+1 for this, i ran into some errors yesterday that were undescriptive and did not tell me what was wrong.
(but i guess it isnt djs so sorry abt that)
manic horizon
cc: @tranquil osprey @restive crown
tranquil osprey
cc: <@268792781713965056> <@139836912335716352>
Now if only you had also already seen the PR and read shapeshift-dev instead of pinging us 8 hours later.
manic horizon
I have seen the PR, and I have read shapeshift-dev
tranquil osprey
Well anyway idk how to improve this in a good way. This is your field tbh Kyra. You designed all of that. 
manic horizon
Making an issue in Node.js to make them use util.inspect on crash errors would be a good start
But also would be handling your errors, I know for a fact that frameworks like Sapphire are very resilient to this kind of crashes
tranquil osprey
@inner gyro 
restive crown
That said we could make error.message be the pretty message we generate for inspects
inner gyro
But also would be handling your errors, I know for a fact that frameworks like S...
I know the pretty solution is to do that but I think you’d agree that throwing a try catch on every djs builder wouldn’t be too practical. My idea is that if you handle stuff properly it will still show up like it used to but if you don’t this gives you a better idea of what went wrong.
olive locust
What about a process unhandledRejection listener or similar to log it then
The problem is partly that Node doesn't util.inspect it's unhandled error logging as kyra mentioned
teal basin
im confused
try catch on every djs builder wouldn’t be too practical
your stack trace shows that this happened within a command
why don't you... try..catch your command.exec call or whatever at the commandhandler level
and you can specifically deal with certain types of errors or otherwise just re-throw and let it crash, if that's what you want
manic horizon
Sapphire basically has a try/catch with import(), so if a module's top level throws, it's handled
That should be pretty much the case for all frameworks
restive crown
anyways
inner gyro
why don't you... try..catch your `command.exec` call or whatever at the commandh...
I do but these errors are the only ones with the errors property that I know of so a general handler wouldn’t quite work on that
Either way idk why that error in particular wasn’t caught but these errors should never make it to production so I think that if they do the bot should crash so you know you have to fix them
mossy adder
Sorry but
but if we're not expecting any errors (which, most of the time, we aren't) getting a crash log like this won't help us fix the issue
whatever really
you don't code "not expecting errors"
you code "handling potential errors"
period
every call that could potentially fail, you handle
inner gyro
every call that could potentially fail, you handle
that's just wasting time. If I was to catch every single builder instance I create or every time I modify it I'd be wasting my time. You have to support situations like this
tranquil osprey
Entitled much
Besides if you set up your code in an efficient way then it's only 1 try/catch block for all the builders and code. For example that's how we do it for Sapphire. Errors just bubble up to a high level.
mossy adder
that's just wasting time. If I was to catch every single builder instance I crea...
handling potential errors in your code is... wasting time now?
sorry but I think you are completely out of line here
have you tried this approach on other languages/packages yet?
I'm curious of their exchange
inner gyro
handling potential errors in your code is... wasting time now?
handling errors that will never happen is wasting time yes. Handling other errors that could happen isn't
inner gyro
they happened once
mossy adder
yeah but they did
mossy adder
so something you were not expecting was happening
mossy adder
no, I don't agree that something should crash without proper logging
if at all, you should exit if it reaches an unrecoverable state
meaning you handle the state and agree it being unreachable, log the error, exit
inner gyro
thats literally what happened
but since i wasnt expecting it i have no way of logging properties hidden underneath
mossy adder
clearly not, because otherwise you would have proper logging in place
inner gyro
i have what the error shows
inner gyro
jesus christ how hard is it to just support more than 1 use case
mossy adder
there is no use case here
mossy adder
no there isn't
inner gyro
mate these errors shouldn't happen in production
so when they happen you're not expecting them
mossy adder
showing X is missing and 3+ others isn't gonna make you fix your problem
you still need to know the 3 others
so you still gotta look at it
inner gyro
showing `X is missing and 3+ others` isn't gonna make you fix your problem
did you look at the fucking PR?
tranquil osprey
I know I'm not a mod but can we chill on the cuss words please
mossy adder
look, this isn't the first validation lib that is doing this
every validation lib has an expandable errors array
where all of them are listed
inner gyro
the first lib you guys used on builders was quite verbose
i dont remember what it was but its only gone downhill from there
mossy adder
uhm, no not really
support was full of questionable errors
and so were issues on the repo
inner gyro
yes with the second one
mossy adder
people asking to "improve it"
inner gyro
not the first
mossy adder
no, with the first one
inner gyro
you changed because they moved to esm
mossy adder
zod was it btw
tranquil osprey
The first was Ow iirc
mossy adder
no we didn't
mossy adder
it still doesn't matter
handle your errors
don't expect some lib to handle them for you
inner gyro
mate its as simple as saying the errors instead of saying "received one or more errors" which is literally the most useless information you could give
if you have to give an initial error message make it useful
if that's gonna be the thing node shows by default dont expect people to ignore it
manic horizon
Bubbling up the message isn't going to bubble up the context as well
That's the reason why I don't really approve your PR
And the reason why it's not dynamic (avoid CombinedError if there's only 1 error) is because the structure should be the exact same no matter what the error is, so developers can pick a specific nested error if they want to
inner gyro
picking a specific error is very hard because you don't know what position it will even be in even if you're expecting it
mossy adder
without context this is not helpful at all thought except in very simple use cases
manic horizon
Also, how are you even loading your code and how are you consistently getting builder errors that make your app crash instead of recovering and sending an error message back?
mossy adder
what if its super nested
what does it matter if it tells me 4 times I passed a string instead of a number
I still would have to check for context
like yeah I could just randomly go through my code and check where I pass a string instead of a number
but thats even more work than just expanding the errors array
and get the exact location
inner gyro
Also, how are you even loading your code and how are you consistently getting bu...
this literally happened once I'm not consistently getting errors
but it was annoying enough that I had to spend 5 minutes trying to understand why it happened
mossy adder
so even with your PR, on complex structures I have to log/expand the errors array
inner gyro
I still would have to check for context
yes but at least you know where to look
mossy adder
and without it... I have to expand/log the errors array
so either way, I have to log/expand it
inner gyro
so even with your PR, on complex structures I have to log/expand the errors arra...
I didn't really
mossy adder
well you didnt
manic horizon
Not really, Crawl already gave you the reason why it won't
inner gyro
because just with the message you can have an idea of what happened
inner gyro
Not really, Crawl already gave you the reason why it won't
won't what
mossy adder
spams your console
what are you gonna "know" from that
you don't know where the erroring keys are
inner gyro
spams your console
well yeah you get 1 line from each error, how is that bad
at least you know what it expected
you know what structure expects what
inner gyro
so you know where to look
mossy adder
you don't know which key, where, which value
inner gyro
you do because you have the stack trace
you have a file and you know what happens in that file
so you know where to look if you know your own code
manic horizon
This is related to how you bubble message but not context, Rodry
mossy adder
you know that file could be like... 5k lines long
manic horizon
Context being the specific assertion that failed
mossy adder
No one is going to read "Expected string but got number"
and starts looking at their objects
you go look at the errors array and get the location
then fix that in your object
inner gyro
you know that file could be like... 5k lines long
well if you have a file that long you're doing something wrong but
if you would allow me to finish
saying a string was expected and instead it got whatever value tells me where to look and what was returned
mossy adder
well if you have a file that long you're doing something wrong but
if you have no proper error logging in your application you are doing something wrong
inner gyro
in my case I didn't know something would return such a big array, so it errored
inner gyro
do you really
you're saying the messages don't help at all
im saying they help a lot
im trying to prove to you that they do
you keep insisting
this isnt going nowhere
mossy adder
they don't help, because in most use cases you need to expand the errors array anyway
manic horizon
Rodry, also, how do you propose we fix that?
mossy adder
expect you like the grunt work of checking everything manually after reading some "error" message that has no context
manic horizon
The inspect override is lazy-executed, called by util.inspect
And it's also Node.js specific
Remember that Shapeshift, just like many other Sapphire projects, need to run in browsers too
inner gyro
Rodry, also, how do you propose we fix that?
I literally made a PR
manic horizon
So running the inspect override even in browsers will break the compatibility
inner gyro
they don't help, because in most use cases you need to expand the errors array a...
who says its most cases? I just showed you a case where I hadn't
mossy adder
ok ill try to bring it down to your level
inner gyro
Remember that Shapeshift, just like many other Sapphire projects, need to run in...
well i dont have a project running on the browser so idek what the errors would look like there
dont ask me to fix that
i proposed a solution to a problem i found
if you have alternatives i can edit the PR
manic horizon
And I'm against it, message alone isn't that helpful compared to the context it comes with
mossy adder
Received 2 errors:
Expected a string primitive
Expected a string primitive
how the in the hell am I supposed to know where, what, how without manually going through my object and almost guessing where I do something like that
while:
1 ValidationError > StringValidator
| Expected a string primitive
| Received:
| | 42
1 ValidationError > StringValidator
| Expected a string primitive
| Received:
| | true
tells me exactly what is going on
inner gyro
And I'm against it, message alone isn't that helpful compared to the context it ...
it's more helpful than literally nothing
inner gyro
```
Received 2 errors:
Expected a string primitive
Expected a string primitive
`...
you know that after all of this comes a stack trace
that tells you where
mossy adder
if you log it properly
manic horizon
The stack trace doesn't tell you where the object was created at, not even a hint
inner gyro
if you log it properly
according to you I didn't "log it properly" and I had a stack trace pointing exactly to the line that errored
it just didn't show me what was wrong
it said it was in the value of an embed
didn't say what it received
manic horizon
You could have methods across 5000 lines of code that are used to create an object, piece by piece, and then send the full result to Shapeshift
inner gyro
You could have methods across 5000 lines of code that are used to create an obje...
yes and the line that errors is thrown into the trace
manic horizon
Without the context, you have not a clue of what might have caused such value to happen
manic horizon
No, because it's called in another place, earlier
inner gyro
yes the one line
mossy adder
but not which values are wrong
inner gyro
well it does tell you what values are wrong
mossy adder
the executing call could be god knows how big inside
inner gyro
lemme bring this back
mossy adder
yes, here I wouldn't know anything, even with your "improved" error message
expect its in help.ts
on line X
but that doesn't tell me what
just where
inner gyro
it says its the value on an addFields call on help.ts line 207
with my PR it would say that it expected a string but received an object
which I wasn't expecting
and that was enough to figure out the error
i didn't even bother to check what it received
wasn't needed
manic horizon
Conditional code exists, you know, if, if else, else, for, while, do ... while, etc, you could be creating an object in many ways from many different places, and the stack is only going to tell you where the main route with the result came from.
Stacks contain the origin to the destination, and the direct path that leads to one to another, but does not include the branches it took to get the values you're passing
Without the context, you'll hardly have an idea of where a value comes from, the message isn't enough to diagnose the issue you're getting
inner gyro
Conditional code exists, you know, `if`, `if else`, `else`, `for`, `while`, `do ...
even if your code is nested inside a loop you would know what you're looping from so you know you have to log that to see what's wrong
manic horizon
I'm talking about functions that run loops, outside of the main branch
inner gyro
yes you still know what function and where it started
it might not be the best clue, sure
but it's something
and you can start debugging there
manic horizon
Not if you're conditionally calling functions based on runtime values
mossy adder
and you can start debugging there
thats like finding a needle in a haystack
inner gyro
it's better than nothing
inner gyro
which is what you have rn basically
inner gyro
yes from the array you can filter out the correct values
inner gyro
you just need to log it
you're literally saying you need to full on ignore the default data NODE gives you
how is that a good workflow?
how is that a good user experience?
how are you gonna deal with this when a guy that started coding yesterday comes at you with 3 of these errors and cant figure out what went wrong?
mossy adder
you tell me how not handling errors is a good dev experience
mossy adder
how are you gonna deal with this when a guy that started coding yesterday comes ...
tell them to log their errors
teach them the basics
as it should be done
inner gyro
you tell me how not handling errors is a good dev experience
you log errors you're expecting, you don't log errors you're not expecting, and specially im not gonna log an error that only exists in shapeshift and not all the other errors I could be receiving
tranquil osprey
you know that after all of this comes a stack trace
Node and browsers also cut off stack traces at some point so if the code is deeply nested then you still wont know where the code came from
inner gyro
logging error.message (what I do) works for all errors because all errors have a message
inner gyro
Node and browsers also cut off stack traces at some point so if the code is deep...
well yeah of course but it's still something
and it gives you the top call
mossy adder
you log errors you're expecting, you don't log errors you're not expecting, and ...
so you are telling me, when you use a VALIDATION library, you don't expect errors?
why not turn off validation
tranquil osprey
The top call isn't necessarily the place where the validation code is
inner gyro
why not turn off validation
well no shit sherlock I wish I could
tranquil osprey
You can
mossy adder
lol
inner gyro
so you are telling me, when you use a VALIDATION library, you don't expect error...
I do expect errors, I just don't log every special property every error could potentially have
mossy adder
I do expect errors, I just don't log every special property every error could po...
sounds like thats on you
tranquil osprey
Have you bothered checking the docs? https://www.sapphirejs.dev/docs/Documentation/api-shapeshift/modules#setglobalvalidationenabled
inner gyro
sounds like thats on you
mate? How do you expect an app with say 10 different runtime dependencies to check all special properties of all errors coming from all packages?
no, you just log the property you know all of them will share
mossy adder
uhm, no not really
inner gyro
and for the cases where that property doesnt exist you log the full error
inner gyro
so if you had a general error handler do you think it's fine to just log error.errors ?? error.code ?? error.res ?? error.message ?? error
inner gyro
Have you bothered checking the docs? https://www.sapphirejs.dev/docs/Documentati...
no, I don't use shapeshift as a main dependency lol
mossy adder
there is no such thing as a general error handler, but yes, if I would go as far and have one (which is still a bit complex for a single method) I would pass all kinds of error interfaces it could expect
and then do this whatever insane nested check
inner gyro
Likewise <https://discord.js.org/#/docs/builders/main/function/enableValidators>
so you're telling me i have to call that at the start of my app to disable validation everywhere
manic horizon
You use builders as a main dependency, and has that global function exported and documented
inner gyro
there is no such thing as a general error handler, but yes, if I would go as far...
that's just not practical man
you can code like that all you want
I don't wanna clutter my code with shit like that
mossy adder
lol
inner gyro
You use builders as a main dependency, and has that global function exported and...
I don't actually
mossy adder
I hope you never work in that field
inner gyro
but sure
mossy adder
sorry but what are we even doing here
it sounds just like complaining about not wanting to handle X errors
inner gyro
all of these discussions come down to a point where there is literally NO downside of applying my change because it just gives more info that you currently get
if you think that's not enough you can log the rest, but for the cases that is it helps
hell you can't even tell if its 1 or more errors
that's not practical
manic horizon
But what if you're picky about the inspection level and set the default to 1?
Then that message wouldn't log at all
inner gyro
how do you change that even
inner gyro
no one is using that here
and no one in their right mind would intentionally make their stack traces worse I think
at least I didn't
and even then with a depth of 1 what would you show? Nothing?
do you just ignore the error
manic horizon
You could change it for general purpose defaults
inner gyro
the message is the most basic part of the error, I don't think you can get rid of that
manic horizon
Obviously for validation libraries it's best to use a very high depth, because that allows you to read a very nested error
You can if it's deep enough
manic horizon
With a more nested schema
manic horizon
You'd be surprised by how many cases we test Shapeshift under
That thing literally has 100% coverage, even the util inspect overrides
inner gyro
well ok, tell me 1 case where this solution is worse than the current one
if its the exact same it doesn't matter does it
inner gyro
and if you get an error thrown would that not show up at the top level?
manic horizon
No
inner gyro
how come
manic horizon
That generates...
manic horizon
- CombinedError (
s.union(s.nullish, x)) - IndexedCombinedError (
s.array(x), will show0in this case) - CombinedError (
s.union(s.nullish, x)) - PropertyCombinedError (
s.object(x), will showfooin this case) - ValidationError (
s.string)
inner gyro
ok and what reason would the top error show?
if an error is nested it would go to the errors array wouldn't it?
and if it goes to the errors array it going be on the error message following my PR
so it still shows the reason why it errored
it literally cannot be worse than it is
i didn't claim it was a perfect solution
i just said its better than the current implementation
where the error message is literally disposable
manic horizon
ok and what reason would the top error show?
One or more errors received
Your patch doesn't fix it btw
And it's going to be laughably bad design if it did
"Expected string", in a schema taking an array of objects
So now you pass a string to parse, and now it says "Expected array"
That's even worse than rn
That's why basically all libraries report nested errors, and Shapeshift is no exception
You gotta log the entire thing, if you bubble up stuff without the context it comes with, you get counterproductive misleading nonsense
inner gyro
So now you pass a string to parse, and now it says "Expected array"
no that's just stupid. you should know what properties expect what in your code
you're not gonna change an array for a string because the error said it expected a string
it's obviously something inside the array that expected a string
manic horizon
Builders aren't your code
And your PR aims to do that, bubbling errors so they can be seen with lower depth
The example above logs the message at depth 4
inner gyro
Builders aren't your code
yes but you know what your code expects
manic horizon
If you bubble it once, it'll be 3, too short from the default, 2
I wouldn't know
For what it's worth
inner gyro
If you bubble it once, it'll be 3, too short from the default, 2
what are you trying to say
inner gyro
I wouldn't know
how come
manic horizon
We could have changed rest params to an array
And then to make your life harder, make the array rest params
And then support the two
We could change our validators in many ways
inner gyro
We could have changed rest params to an array
yes and types would change
and there'd be something in release notes
you'd have to know somehow
you can't be coding with a package you don't know
manic horizon
For what's worth, you could be passing an object typed as any
Seeing you had issues with TypeScript recently
And that would have voided the type safety
Also not applicable to JS users without typechecks
inner gyro
well mine was yeah I'll give you that, but still saying what was expected helped figure out what happened
it's as simple as
it says what error it got
if thats not enough you log the rest
if it is great, you can fix the error
manic horizon
You know
You just gave your own answer
It's as simple as
It says what error it got
If you log it, you'll get the details
If it's great (and most likely is), you can fix the error
lavish bolt
you're not gonna change an array for a string because the error said it expected...
uh, no, if something says it expects a string it expects a string
what that should say is it expects an array of strings
inner gyro
If you log it, you'll get the details
yeah what if you don't log it ffs you keep coming back to the same thing
inner gyro
uh, no, if something says it expects a string it expects a string
what that shou...
yeah no it doesn't do that
lavish bolt
if you don't log it you don't get the details, shouldn't that be obvious
inner gyro
dont add more confusion
inner gyro
if you don't log it you don't get the details, shouldn't that be obvious
yes you get the default error
which should give you some information
and currently doesnt
thats the entire point of this
lavish bolt
which should give you some information
pretty sure it does, tells ya there's an error with combined properties and validation, even tells you where
on like 207 of your file specifically
i agree it could be more helpful but it currently does give you information
inner gyro
it tells you there's one or more errors, it doesn't tell you what they are
in my case it expected a string and got an object, I wouldn't know that from that error alone
lavish bolt
it tells you there's a combined property error and a validation error literally in your screenshot
but yes it could say more
lavish bolt
link me to your pr so i can take a look at your solution
tranquil osprey
link me to your pr so i can take a look at your solution
lavish bolt
yeah error message alone is not a good way to go about it
if you want comprehensive coverage it should tell you what it expected and its type, what it got and its type, and the type of error that created
not just "Expected XYX" but more like the example given by Crawl
and i'm pretty sure that's down to the error handler / logger since the error array it throws contains that information
so realistically use a different error handler or complain at node for their default logging
inner gyro
if you want comprehensive coverage it should tell you what it expected and its t...
the example crawl gave is what the errors array includes. Putting that on the error message is too much and that would actually clutter the console
this solution is meant to be simple, not replace the errors array
lavish bolt
while "Received one or more errors" isn't helpful, there isn't much else it can say when it produces a list of errors without flattening nested arrays to their first instance of a non-array element and returning those errors
tbh I would do this:
- if there's only one error contained at any level, throw it with some context about the original container error type
- if there's multiple nested, find the first array in the nest with more than one contained element and throw that instead
- if not, keep it as is
tbh if i were you i'd just use a different logger to the default that deals with all these situations better
would be easier
although you could add another check to compress the error messages into one titular message with the thrown array if all the errors are of the same type
inner gyro
I don't wanna change the way errors are thrown, I just wanna make a useless message useful
that's literally all
you can't say that message isn't useless, because it is
lavish bolt
aye, the only way to make it meaningful is to do what i just said afaik
or AT MINIMUM use "Expected an X, received a Y"
expected an X is equally useless and if anything brings about more questions
also if there's an array of errors you absolutely cannot just use the first error message unless there's a guarantee about the order of the errors in the array, that's nonsensical
inner gyro
or AT MINIMUM use "Expected an X, received a Y"
thats what it does
lavish bolt
nay, it does not
it just says "Expected an X"
in your defense that's a mistake that has perpetuated from its original design but i think it's within scope to rectify it here
inner gyro
like I said, I don't wanna change anything else, only this error message
i didnt design it, ill leave the other error messages for others
lavish bolt
we can't just replace one bad solution with another
if we're fixing it we gotta fix it
hidden cypress
wait but, what are you fixing again?
lavish bolt
so, I'd say we should repurpose the PR to refactor error messages altogether
then, we can replace the "Expected an X" with "Expected an X, received a Y"
and then we can deal with the reporting of combined errors more effectively
olive locust
"Fixing" what the library outputs when an error message is logged by Node's default unhandled exception logger due to an absolute refusal to even consider the possibility of catching and handling errors in your own code
Even though you could just process.on('unhandledException', console.error) and then it will use util.inspect and you'll get better output
lavish bolt
you're being defensive now, that's not the intention of the PR and we both know it
hidden cypress
it really is
the PR exists because of the reasons in this thread that were summed up above
lavish bolt
work with, not against - Rodry is somewhat right, saying "There has been an error" in an error message is genuinely quite useless
lavish bolt
no it's useless no matter what
saying there has been an error in an error message is a redundancy no matter what information is attached to it, you may as well just have the info with no message
when was the last time you saw an error message that was clearly an error message already that felt the need to inform you there was an error
inner gyro
"Fixing" what the library outputs when an error message is logged by Node's defa...
I never refused to log anything, I'm saying this for the cases that happen once and you're not expecting them and the only thing you can work with is a crash log from node
hidden cypress
the last time i got a combinederror from shapeshift
lavish bolt
lmfao
inner gyro
it's useless because he doesn't log it
nothing in an error should be useless. period
hidden cypress
but since i logged it, i looked at the deeper stuff
inner gyro
an error is meant to give you info
lavish bolt
nothing anywhere should be useless on that note
hidden cypress
and fixed it in one go
inner gyro
an error saying there's an error is like saying the floor is made out of floor
lavish bolt
but, yeah, while the motives might not be correct on Rodry's end, it is important to use the opportunity to rectify some questionable things in the way those errors are dealt with
hidden cypress
that's just wasting time. If I was to catch every single builder instance I crea...
and yet this is a comment
inner gyro
yeah so what
lavish bolt
"while the motives might not be correct"
read the sentence once more please
and once you are satisfied with the comprehension, we may continue
inner gyro
All I said is that coding everything to catch every little error that could potentially occur is dumb but realistically won't 99% of times. If you get an error consistently it's worth adding a log to investigate further. If you can't reproduce an error you'd only have the default crash log, which isn't too helpful
lavish bolt
you are categorically incorrect and I'm only one sentence into reading it
hidden cypress
it's not dumb. it's how that works, given how the code that throws is literal validation lib
no one says to catch yoru Math.floor() really
lavish bolt
code must always be safe, errors should always be caught
hidden cypress
by you, mind you, not node's recovery
lavish bolt
i implore you to research more about effective error handling
inner gyro
yes I catch my errors, I just don't check a property that only exists in the errors coming from 1 package
I check the properties that all errors have
so it works for all situations
this is never a problem with any other package
lavish bolt
it is literally a problem with any package that produces nested errors but okay
inner gyro
which in my app is only 1 and this was the first time i got an error like that in 2 years of running it
lavish bolt
i was defending you until you started being incorrect again
your responsibility as an engineer is to deal with problems correctly, ours as engineers is to make sure the way we report those problems is effective
inner gyro
and the way this particular error is reported at the moment isn't effective
lavish bolt
that's quite literally the point i made earlier
inner gyro
well yeah
lavish bolt
you satisfy your duties, we'll satisfy ours
so, here's my proposed solution
you fix your error logging mechanism
we (or at least I) will make the error messages better
sound good?
lavish bolt
at the basic level, "Expected an X, received a Y"
at the more sophisticated level of combined reporting, that's a matter for discussion
inner gyro
and does that include getting rid of "Received one or more errors"?
lavish bolt
yes, I've already said that statement is redundant
I've helped with translations in Sapphire and linguistics for contributions in PRs and documentation for a long long time
you may ask Kyra and Favna if you wish to hear about my track record
but for now, trust me, you're in safe hands
inner gyro
well alright do you wanna make a PR then
if you open one ill close mine
and watch yours
lavish bolt
no I'll just stem from yours
inner gyro
you want access to my branch or what
lavish bolt
you can either implement our devised suggestions or add me as a contributor to your fork to make that work
inner gyro
sure then
lavish bolt
up to you, I can either make suggestions on the PR or commit directly, that's up to you
inner gyro
ill add you, whats your GH?
lavish bolt
same as my Discord
inner gyro
done
lavish bolt
thanks, I'll get to work shortly, shouldn't take too long, it's not a big change
inner gyro
alright, thank you
lavish bolt
I'm not sure of the most appropriate way to summarize the error messages in a collective context tbh
could be a duplicate-free list of the error messages, could be a list of how many of each kind of error there was, etc
that's up for discussion
to help with spam duplicates can be replaced with numbers, like
"Encountered errors: (2) expected a string but received a number, expected a string but received an object, (3) expected a string vector but received a vector containing a number"
which is imo much neater than
"Expected a string, received a number
Expected a string, received a number
Expected a string, received an object
..."
inner gyro
to help with spam duplicates can be replaced with numbers, like
"Encountered err...
If you can get that to work without a lot of complexity ye that’d be nice
Would help the spam
lavish bolt
dynamically determining which indefinite article to slap on a type is a horrendous feat given english and its non-phonetic alphabet, any chance we can ditch em all together
we get bonus brevity if that helps justify it
like "expected string but received number"
still (mostly) grammatically correct, i doubt anyone will lose sleep over it
it just saves added complexity of determining which indefinite article to use and also saves the headache of giving them all a fixed indefinite article
i'd be more annoyed with reading something like "expected a string but received a array" or "expected a string but received an number" than just "expected string but received number"
or if you guys are extra pedantic and don't mind verbosity we could use "Expected an instance of X but received an instance of Y"
manic horizon
@lavish bolt mind you, such calculations may make Shapeshift's errors slower than they are, there's a reason why I keep the error system simple
And as soon as you make CombinedError inconsistent with the others, the error API will be terrible, which is why I'm very against Rodry's PR
When using console.log or similar, you're util.inspect'ing Shapeshift errors, which in turn become much richer and far easier to read
inner gyro
And as soon as you make CombinedError inconsistent with the others, the error AP...
CombinedError is the only error that throws that message, so its not more inconsistent than it already is
lavish bolt
<@699399006715117629> mind you, such calculations may make Shapeshift's errors s...
do they already have performance issues at present?
i haven't been able to test that so forgive me for my blissful ignorance lmao
although, like Rodry said, it's already inconsistent, since most of the error types are not nested
CombinedError just needs to reflect the rest
at minimum we need to change the error messages given by the other error types
i'll take a look at where CombinedError is thrown to measure its performance impact
although it does concern me that judging by the error message it is thrown even when there's only one error
manic horizon
Rodry's "solution" isn't going to solve anything
His main premise is that the inspection depth is too low to get such information
But we recommend giving it a high depth anyways
inner gyro
His main premise is that the inspection depth is too low to get such information
No that is not my main premise did you read this convo at all?
My point is that the message is useless
I want to make it not useless
It’s as simple as that
manic horizon
I stand by the fact you did little to no research at all, Rodry
When you called CombinedError unique and thus "it won't become inconsistent", when there's this, literally the same error message and nearly the same structure:
https://github.com/sapphiredev/shapeshift/blob/main/src/lib/errors/CombinedPropertyError.ts#L8
We're looking into fixes for the issue, but so far nothing improves the current one, your PR is merely a bandaid that like all the other solutions, don't solve the issues at hand
On most cases, your solution will be that, you can even see where CombinedPropertyError is used
Now, if you make the error messages bubble up, even for CombinedPropertyError so there's consistency, they'll be incredibly misleading, and won't account for CombinedError within CombinedPropertyError, as shown above
inner gyro
When you called CombinedError unique and thus "it won't become inconsistent", wh...
yes I fucking changed that what
inner gyro
On most cases, your solution will be that, you can even see where `CombinedPrope...
ive never seen good code practices recommend the use of util.inspect actively, you're the first one ive seen insist on that so much
manic horizon
Because console.log already does inspect for you
inner gyro
yeah...
restive crown
Ok Rodry, I get there is a clash of thoughts/opinions but do try to keep it civil please
Anyways as previously said, changing the error.message just in the event theres one error is not gonna fix the issue at its roots
inner gyro
Anyways as previously said, changing the error.message *just* in the event there...
once again, did you look at the PR?
that's not what I did...
I did that at first but then changed it
restive crown
Still doesn't fix it rodry...
What should be done is making error.message a getter that generates the in depth messages that util.inspect does
inner gyro
What should be done is making error.message a getter that generates the in depth...
that would make it huge
it cant be that big
restive crown
Why not
inner gyro
its supposed to be a summary of the error
restive crown
No..
inner gyro
Node says "The error.message property is the string description of the error as set by calling new Error(message)."
its a description of the error
should be simple
not 20 lines long...
restive crown
Ok let me put it this way
There's no length limit. This long message is the description of the error
inner gyro
Also *your* PR
yes that's not long
there's 3 errors
there's 3 lines
its not 10 lines per error
which is what you're proposing
and I think that's worse
manic horizon
I could have done 10 errors, so it's 10 lines long
inner gyro
because then it defeats the purpose of the errors array
inner gyro
I could have done 10 errors, so it's 10 lines long
yes exactly
but its not 10 lines for 1 error
manic horizon
You're contradicting yourself
inner gyro
how am I contradicting myself
manic horizon
You demand better errors because you don't want to catch your errors, and at the same time, when Vlad proposes an approach that also solves the issue, you also reject it
inner gyro
You demand better errors because you don't want to catch your errors, and at the...
vlad's proposal makes it worse
you don't want your console spammed with errors
unless you get tons of errors
wintry elm
I definitely want my console to show as much detail for an error as I can. If you don’t want your console to „spam“ errors catch them properly and handle them. For any unexpected error you definitely want it to not only show one line but as much information about it as possible. So if you feel that is spamming your console instead of providing good intel to solve the problem you definitely have a wrong approach to error handling.
inner gyro
An unhandled error should give you enough information for you to fix it. That doesn’t necessarily mean it has to make it pretty with all those bars and new lines the errors come with, it’s as simple as putting the message and, if anything, include the expected and received values but not in a way that spams the console completely unnecessarily
lavish bolt
I definitely want my console to show as much detail for an error as I can. If yo...
it should be concise, not as verbose as possible, to clarify
as much rich information in a small space as possible
manic horizon
Funny that you say that, coming from a Rust developer
And each error is 5 lines, not 10, displays the kind of error, the validator (or constraint that triggered), the message explaining what happened, the expected (if any) and the given value
Mind you, we're not talking about your average Java exception, however, JS's exceptions are pretty huge, and it doesn't help that the crash reports don't respect errors util inspect
Let me demonstrate
This is how a crash log looks like, using console.dir, which inspects but blatantly ignores Error's overrides
(And yes, that's missing a } at the end, in a 2160p fullscreen window, it just doesn't fit with my current font size)
This is what you get if you util inspect:
Fits in a 1080p fullscreen window, and has space for more lines
You don't want your console flooded? Then log the error, and not only it'll become easier to read, it'll also be shorter