fix: improve excel2xml (DEV-1361) #232
Conversation
…into wip/dev-1361-improve-excel2xml
| warnings.warn(f"Date parsing error in resource {calling_resource}: '{iso_date.group(0)}' is not a valid " | ||
| f"date", stacklevel=2) |
There was a problem hiding this comment.
I don't think I understand what this warnings thing is. As it's removed here, it's not so important... but if you want to say something about it, I'd be curious
There was a problem hiding this comment.
I use warnings in some other places in dsp-tools. At the long term, I try to implement a system as described here: https://docs.python.org/3.9/howto/logging.html#when-to-use-logging.
The idea is that not every issue is severe enough to raise a BaseError, for example when a file path of a is not valid (in a development context, it is possible that the original files aren't available yet). Especially when Vij uses excel2xml in the command line, he is not able to catch Errors. Then it's better if the warnings are printed, without quitting the program.
But overall, I feel very unsure about how to treat Errors. I'm still looking for an overarching architectural concept, so that I can do it consistently, everywhere in dsp-tools.
There was a problem hiding this comment.
wanting to implement a proper logging system is something I can absolutely get behind (and that I have been mentioning as "should do eventually" since I started working on DSP-Tools).
But I have to admit, I didn't even know that warnings.warn() exists. Interesting to know indeed, however I think in our case, using logging.warn() seems more what I would expect - from what I understand, warnings is more intended for libraries that expect to be used by other libraries, to pass on information like deprecation warnings, etc.
But yes, error handling in DSP-Tools is not an easy subject, that's for sure.
There was a problem hiding this comment.
Switching to the built-in logging module is on my list of todos, and I have already done some reading about it.
When I chose warnings.warn(), I didn't have the CLI version of excel2xml in mind, but rather the importable module. I had then the feeling that warnings would be the right choice.
But this needs to be definitively decided when I switch to logging in the entire dsp-tools repo.
resolves DEV-1361