Convert `can.Logger` and `can.LogReader` into functions by zariiii9003 · Pull Request #1703 · hardbyte/python-can · GitHub
Skip to content

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

Merged
zariiii9003 merged 9 commits into
hardbyte:mainfrom
zariiii9003:refactor_io
Jan 14, 2024
Merged

Convert can.Logger and can.LogReader into functions#1703
zariiii9003 merged 9 commits into
hardbyte:mainfrom
zariiii9003:refactor_io

Conversation

@zariiii9003

@zariiii9003 zariiii9003 commented Dec 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator
  • 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
lumagi
lumagi reviewed Dec 17, 2023
View reviewed changes
Comment thread can/io/logger.py Outdated

@lumagi lumagi Dec 17, 2023

Copy link
Copy Markdown
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?

@zariiii9003 zariiii9003 Dec 19, 2023

Copy link
Copy Markdown
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.

Comment thread can/io/logger.py Outdated
File will automatically recompress upon close.
"""
Logs CAN messages to a file.
real_suffix = pathlib.Path(filename).suffixes[-2].lower()

@lumagi lumagi Dec 17, 2023

Copy link
Copy Markdown
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.

@zariiii9003 zariiii9003 Dec 19, 2023

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

Comment thread can/io/player.py Outdated
Comment thread can/io/player.py Outdated
@grant-allan-ctct

grant-allan-ctct commented Dec 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

zariiii9003 commented Dec 19, 2023

Copy link
Copy Markdown
Collaborator Author

@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

Reviewers

@lumagi lumagi lumagi left review comments

Assignees

No one assigned

Labels

file-io about reading & writing to files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zariiii9003 @grant-allan-ctct @lumagi