move the xml_to_dict and msfiletime to datetime from bruker parser to utils.tools

[sem-geologist]

Description of the change

There are some dublication attempts spotted (i.e. XML handling) where different kind of
parsers have its own implementation.
This PR moves some of functions from bruker api to utils.functionaly_coresponding_file:

• move and expose msfiletime_to_unix (datetime.datetime) in utils.date_time_tools
• prelude to unification of XML to dictionary translations: move and refactor customizable xml to dictionary translator from bruker api:
  • float sanitizer function
  • xml dictionarization

This is first step. Next step (other PR) will be format by format review if XML can be parsed using the same tools.

Progress of the PR

• msfiletime_to_unix
• move xml helper tools to utils.tools, make it customizable for reusing and switch bruker api to use that.
• functions contains rich docstrings (done), update user guide (after wider adoption of xml_to_dict translator to other formats in other PR)
• add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
• Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
• add tests, (there is no needed, as it still do same job)
• ready for review.

[codecov]

Codecov Report

Patch coverage: 95.74% and project coverage change: +0.05 🎉

Comparison is base (85918a4) 85.20% compared to head (9092c19) 85.26%.

❗ Current head 9092c19 differs from pull request most recent head ef37b8d. Consider uploading reports for the commit ef37b8d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   85.20%   85.26%   +0.05%     
==========================================
  Files          73       73              
  Lines        8991     9030      +39     
  Branches     2030     2045      +15     
==========================================
+ Hits         7661     7699      +38     
  Misses        871      871              
- Partials      459      460       +1     

Impacted Files	Coverage Δ
rsciio/utils/tools.py	80.30% <95.31%> (+6.96%)	⬆️
rsciio/bruker/_api.py	88.11% <96.42%> (-0.18%)	⬇️
rsciio/utils/date_time_tools.py	93.87% <100.00%> (+0.26%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[sem-geologist]

Ok, this is kind of ready to review, few minor issues which I had spotted:
There is no test file for utility.functions, As I moved some function there I had to leave testing of that universalized function inside test_bruker.py.
Do I need to add any changelog? this does not change anything for users. This makes tools available for developers.

[ericpre]

Would it make sense to write some test specific to the cases/issues it need support for? This would make future support and development much easier.

Lint is failing. For what it is worth, pre-commit configuration have been added recently in #86.

I would add a changelog, because the changelog is also useful for developers, for example, to easily find when things have been done, the context, reasoning, etc.

[sem-geologist]

@ericpre, I find current lint rules to be stupid and offensive. It asks to change better formatted (more readable) code into worse formatted (less readable)! I still use older hardware for most of my work (it is not even 2k, just old 1k laptop widescreen of good old thinkpad T420), lint complains about line formatted to fit to 79 charter and want to collapse it into single line much over 80 charters. That makes it pain to have two split files opened side-by-side. One is to allow writing longer lines (up to 88, seriously?) which should be allowed exception (best would be to 120) not the rule, but forcing collapsing divided lines into single cluster***** line - that is offensive and makes it less readable - absolutely counter-productive. Who let in the Java devs setting this python lint rules?

After looking around internet what this black is I see I am not the only skeptical about it. I understand it forces unified style (mediocre, not very readable but unified). Why pep8 is not enough, most of us here are quite seasoned python devs?

[ericpre]

Regarding the usage of black, I was also a bit skeptical initial for the reasons you mentioned but after using it for some time, I find it good. A lot of these things are matter of preference but it does a good job at keeping the code consistent.

What do you think about writing test specific to the cases/issues it need support for?

[sem-geologist]

I think discussion section will be right place to discuss further about black.

I will add small made up XML and tests. I think it would be good idea to create test_utils.py and put it there as also move some generic tests from test_bruker.py there.

edit: I see that there is test_utils.py under utils subfolder in tests; How had I not noticed that... I will use that for tests.

[ericpre]

Commit 44df588 can be removed now that ipython 8.13.0 have been yanked and 8.13.1 released.

[sem-geologist]

All tests are passing, I think this is ready for review