refactor(utils): simplify configuration validation and error handling - #4240
Conversation
4fe7ca6 to
bd1b3b3
Compare
have we a rule for this? I’ve always avoided the "function" style because I thought the "=>" syntax was the more modern one. If I search the source code (prior to this PR) for I can go ahead and merge this as is, but wouldn't it be worth settling on a single variant globally? |
|
The arrow style insures the context is set correctly, particularly on callbacks. ( timeout or interval), |
No, we don't. I'll look into which ESLint rules could help us achieve more consistency here.
Yeah, it is more modern, but I find it a bit less intuitive. Maybe it's just time for me to get used to it 😅
The numbers, together with both of your preferences, are enough for me to stick with arrow functions. I’ll update the PR accordingly 🙂 Thanks for the feedback! |
bd1b3b3 to
5a9e37e
Compare
I ended up going down a bit of a rabbit hole with this PR. The main goal throughout was to make the code easier to understand and maintain. There are no intended changes to the normal application behavior. The error handling is also more consistent now, with error messages being passed to the point where they are handled instead of being logged in multiple places.
Please let me know if the PR has grown too large or if I have taken things in a direction that would be worth discussing 🙂
Commit 1
I started by looking at
checkConfigFileand felt that it was doing too many different things. I split its responsibilities into a few smaller functions to make the overall flow easier to follow.Outcome: clearer responsibilities and a simpler configuration validation flow.
Commit 2 (edited)
After your review comment, I decided to undo the arrow-function replacement but keep the documentation improvements instead.
Outcome: clearer JSDoc documentation while keeping the existing function style consistent with the rest of the project.
Commit 3
Finally, I noticed that some validation paths were throwing errors without any message:
An error without a message is not very helpful for debugging. This led me to move the error logging to the places where the errors are handled and to make sure that every
ConfigErrorcarries a useful message.Outcome: more consistent error handling and cleaner error output.
Commit 4
After looking at the remaining usages, I noticed that
getAvailableModulePositionswas only a thin wrapper around the internalmodulePositionsarray. It's not part of the documented API for third-party modules, so I removed it and letmoduleHasValidPositionusegetModulePositionsdirectly.Outcome: less indirection and a smaller internal API, without changing the intended behavior.