Refactor clean up logger #482
Conversation
There was a problem hiding this comment.
Less repetitive code would definitely be welcome, just need to make sure the TypeScript types don't get worse :)
Also if you hadn't noticed already... game sim code is probably the worst code in the game. I think two reasons... it's the oldest part of the codebase, and I didn't abstract it well enough when making other sports so often fixing things needs to be done 4 times! There's been a little improvement over the years, but it's still not great.
|
Addressed comments, and made a decent amount of changes to the types... is a larger PR, and I didn't see any pre-existing tests for these changes. If needed I can try and write some tests for the logger and check if the functionality is all there? |
| }); | ||
| fgMissLogType = g.get("threePointers") ? "missTp" : "missTpFake"; | ||
| } else { | ||
| throw new Error( |
There was a problem hiding this comment.
Probably should not crash the browser if log doesnt exist... maybe better to just do console.warn?
There was a problem hiding this comment.
It's actually good because people will notice and it'll get fixed quick. Console messages will get ignored, but could indicate bugs.
For stuff like this I do throw new Error("Should never happen"); so it's not wasting bytes on custom error messages that are likely never displayed. The error in the console (or in my error logger) will include the file and line number.
There was a problem hiding this comment.
okay sounds good, and quick question, is storage not very readily available for error logging? Is that a design constraint to keep in mind when writing new code/logs?
There was a problem hiding this comment.
That's not about storage, it's about bundle size. Of course it's a very small difference, probably doesn't matter! But it's also about not having to spend mental energy on writing nice error messages for a case like "this is never going to happen, and maybe I'm just writing this to tell TypeScript that it's never going to happen".
Overall, I guess it's just personal preference and consistency.
There was a problem hiding this comment.
Interesting! I think its a bit different at work, where we write a ton of logs and description in logs. I think its also because this is primarily a solo dev project, as opposed to one where there's multiple teams working on multiple repositories. But good to know!
| }); | ||
| fgMissLogType = g.get("threePointers") ? "missTp" : "missTpFake"; | ||
| } else { | ||
| throw new Error( |
There was a problem hiding this comment.
It's actually good because people will notice and it'll get fixed quick. Console messages will get ignored, but could indicate bugs.
For stuff like this I do throw new Error("Should never happen"); so it's not wasting bytes on custom error messages that are likely never displayed. The error in the console (or in my error logger) will include the file and line number.
Refactor typing on playByPlay logger to reduce amount of code reproduction. however, caused some initial bugs, still working on this. wanted to keep this branch open just to see if you were open to this, as this is somewhat big? And doing this would imply you would want more changes to typing for the logger, which could cause issues if not done correctly