BUNPC / pysnirf2

Python package for reading, writing and validating Shared Near Infrared Spectroscopy Format (SNIRF) files
GNU General Public License v3.0
15 stars 11 forks source link

BUG: Fix bug with logger call #39

Closed larsoner closed 1 year ago

larsoner commented 1 year ago

Not sure how to add a test since it seems to only come up when using pytest and you use bare unittest, but when adding some code to MNE-Python I got:

mne/io/snirf/tests/test_snirf.py:504: in test_birthday
    a.save()
../pysnirf2/snirf/pysnirf2.py:6117: in save
    self._save(self._h.file)
../pysnirf2/snirf/pysnirf2.py:5879: in _save
    self.nirs._save(*args)
../pysnirf2/snirf/pysnirf2.py:1361: in _save
    self._order_names(h=h)  # Enforce order in the group names
../pysnirf2/snirf/pysnirf2.py:1280: in _order_names
    self._cfg.logger.info(
/usr/lib/python3.11/logging/__init__.py:1849: in info
    self.log(INFO, msg, *args, **kwargs)
/usr/lib/python3.11/logging/__init__.py:1887: in log
    self.logger.log(level, msg, *args, **kwargs)
/usr/lib/python3.11/logging/__init__.py:1559: in log
    self._log(level, msg, args, **kwargs)
/usr/lib/python3.11/logging/__init__.py:1634: in _log
    self.handle(record)
/usr/lib/python3.11/logging/__init__.py:1644: in handle
    self.callHandlers(record)
/usr/lib/python3.11/logging/__init__.py:1706: in callHandlers
    hdlr.handle(record)
/usr/lib/python3.11/logging/__init__.py:978: in handle
    self.emit(record)
../virtualenvs/base/lib/python3.11/site-packages/_pytest/logging.py:372: in emit
    super().emit(record)
/usr/lib/python3.11/logging/__init__.py:1118: in emit
    self.handleError(record)
/usr/lib/python3.11/logging/__init__.py:1110: in emit
    msg = self.format(record)
/usr/lib/python3.11/logging/__init__.py:953: in format
    return fmt.format(record)
../virtualenvs/base/lib/python3.11/site-packages/_pytest/logging.py:136: in format
    return super().format(record)
/usr/lib/python3.11/logging/__init__.py:687: in format
    record.message = record.getMessage()
/usr/lib/python3.11/logging/__init__.py:377: in getMessage
    msg = msg % self.args
E   TypeError: not all arguments converted during string formatting

This fixes the error with the *args being passed to logger.info by constructing the entire string.

sstucker commented 1 year ago

I will merge and release this fix along with a fix to #38

I'm worried I used logging incorrectly and pysnirf might not play nicely with other software that uses it. I am considering implementing the code suggested in #37, but also maybe just removing the logging functionality from pysnirf altogether

larsoner commented 1 year ago

@sstucker assuming your test coverage is good enough you could smoke out logging issues by using pytest to run your tests. As a side benefit you get a lot of cool stuff for free for testing

larsoner commented 1 year ago

... if you want I'm happy to make that transition. And I can also fix the unit testing that appears to be broken

sstucker commented 1 year ago

Something changed on GitHub's end that broke the testing. I've fixed it I think, try and rerun your check.

I would welcome the switch if you're convinced it's a good idea. I didn't have much experience with Python testing or logging when I built what you see here.

larsoner commented 1 year ago

Done @sstucker , ended up needing to make a bunch of little changes/fixes here and there, feel free to look at the diff and they're not explained well enough in comments let me know and I can add more comments or just reply to comments on GH

sstucker commented 1 year ago

This looks good! Do MNE folks need this released ASAP?

larsoner commented 1 year ago

No, I hacked in a monkeypatch.setattr

https://github.com/mne-tools/mne-python/pull/11912/files#diff-ec85a182541bb4e4305a5b5aea227b7488035b48b61338febe9505d5fd05ece2R500