8000 Convert `can.Logger` and `can.LogReader` into functions by zariiii9003 · Pull Request #1703 · hardbyte/python-can · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Convert can.Logger and can.LogReader into functions #1703

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

Merged
merged 9 commits into from
Jan 14, 2024

Conversation

zariiii9003
Copy link
Collaborator
@zariiii9003 zariiii9003 commented Dec 7, 2023
  • convert can.Logger and can.LogReader into functions
  • split listeners.rst into notifier.rst and file_io.rst.

@zariiii9003 zariiii9003 added the file-io about reading & writing to files label Dec 7, 2023
@zariiii9003 zariiii9003 marked this pull request as ready for review December 12, 2023 17:07
@zariiii9003 zariiii9003 requested a review from lumagi December 12, 2023 17:07
can/io/logger.py Outdated
def _get_logger_for_suffix(suffix: str) -> Type[MessageWriter]:
try:
logger_type = MESSAGE_WRITERS[suffix]
if logger_type is None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can a None value make its way into the dictionary? Is there a root cause that would help the user mitigate the problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already there since the mf4-PR, but you're right, it's not necessary. I removed it.

can/io/logger.py Outdated
"""
Logs CAN messages to a file.
real_suffix = pathlib.Path(filename).suffixes[-2].lower()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw an IndexError if the filename does not have the necessary number of suffixes. Might be better to catch the case and raise an appropriate message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a check for the the length of suffixes

@grant-allan-ctct
Copy link
Contributor
grant-allan-ctct commented Dec 17, 2023

The front page of python-can in github gives this code in the Example usage section:

   # or use an asynchronous notifier
   notifier = can.Notifier(bus, [can.Logger("recorded.log"), can.Printer()])

so we might have a lot of people building their code on top of the idea of the can.Logger being something that can be added to the list of Listeners. (I am one.) Will this still work as is ? If not, the example might need a tweak, and many folks might be interested in how to achieve the same outcome after this PR lands.

@zariiii9003
Copy link
Collaborator Author

so we might have a lot of people building their code on top of the idea of the can.Logger being something that can be added to the list of Listeners. (I am one.) Will this still work as is ? If not, the example might need a tweak, and many folks might be interested in how to achieve the same outcome after this PR lands.

It still works the same way as before. can.Logger was always used to dispatch an instance of another class.

@zariiii9003 zariiii9003 merged commit ec105ac into hardbyte:main Jan 14, 2024
@zariiii9003 zariiii9003 deleted the refactor_io branch January 14, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file-io about reading & writing to files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0