Reducing complexity of optionConverter #2677
Conversation
- moved fallback handling here - expose function processKey instead of object - used import type to prevent circular dependency
|
@Eonasdan This should be substantially done, but I am moving this to draft due to the lack of tests. If possible I would also appreciate a preliminary feedback on my chosen solution, since I have seen other points of the code which would benefit from a similar refactoring. |
|
Wow. Thanks for this. As far as tests go, no. It's been on my todo list as well. I wanted to reach 6.0 release before bothering with that. The one "test" is just to make sure I don't break the TS imports lol. |
|
We all know testing is not easy. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Thanks again. Do you have a good TS testing framework? |
|
Unfortunately no and I acknowledge this is a problem. I would rather have someone more knowledgeable chime in. |
|
@m-gallesio I added vitest and built tests around the DateTime object! It's pretty easy to use. I'm hoping to start building tests around the rest as I can |
Please check if the PR fulfills these requirements
developmentbranchThere is no test infrastructure ready, I ran some manual checks and they were successful.
Not applicable, these are internal refactorings.
What kind of change does this PR introduce?
This restructures the option validation code to reduce its complexity.
The single
switchstatement was split to separate properties of an object.Some of its branches were almost identical; they have been unified by extracting their common behaviour in parametrized helpers.
A small bug was also fixed where 24 would be considered valid in
enabled/disabledHoursdespite not doing anything.What is the current behavior?
See this project item which I mentioned here.
Does this PR introduce a breaking change?
As of now, it does not: since the methods I split off from optionConverter were public I kept them, redirecting to the split implementations.
I think this could be reconsidered, depending on how important they are to expose.
Due to these doubts, I have NOT updated the type definition files yet.
Other information
See these blog posts for the file splitting procedure I used: