conv: use errors instead of logger for convertors#60
Conversation
I'm fine with this and I agree that it's a better experience to see all the problems in the source from the first run so one can fix everything before trying again. What I (and others) have pushed back on before was the idea of falling back to default values when something is invalid and only producing warnings. But this is good. I like this. |
The exporter and importer types have a logger that is used for issuing warnings and errors. The idea was to continue conversion even if some parts of the blueprint were incorrect.
|
Joining the errors together like this is fine in my book; I like to see them all so I don't have to go back and forth all the time. |
The exporter and importer types have a logger that is used for issuing warnings and errors. The idea was to continue conversion even if some parts of the blueprint were incorrect.
This patch changes that for the exporter to return errors instead of logging them. The importer will be changed in a follow-up patch.
I am taking a hybrid approach with errors - errors are collected in the two main functions (top-level exporting function and customization function) and joined using
errors.Join. This means is when an error occurs, the process will continue and finish off the conversion and then the error(s) are returned. Also I make an effort to return data that are still valid in case something is going on, for example inexportUserCustomizationwhen converting more than one ssh key (or keyboard layout).I just cannot get rid of a feeling the convertor should still be a little bit of a "code compiler" experience - when it encounter one error, it does not mean it stops parsing. Modern compilers will go further and try to identify as many problems as possible as long as the error is not fatal (like a missing bracket).
Anyways, if you insist on a strict approach, and something is telling me you will, then I will simply change the code to just return single error and do normal Go error checking everywhere. This will probably render all "invalid" conversion testdata a bit irrelevant - it will always render to one error which is fine I guess.