Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Fix bug with logger call #39

Merged
merged 14 commits into from
Aug 24, 2023
Merged

BUG: Fix bug with logger call #39

merged 14 commits into from
Aug 24, 2023

Conversation

larsoner
Copy link
Contributor

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
Copy link
Contributor

sstucker commented Aug 22, 2023

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@sstucker
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@larsoner
Copy link
Contributor Author

@sstucker sstucker merged commit 74242b7 into BUNPC:main Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants