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

can.io: Add preferred mode for opening files to Reader/Writer classes. #1585

Merged
merged 1 commit into from
May 16, 2023

Conversation

ro-i
Copy link
Contributor

@ro-i ro-i commented May 3, 2023

Hi :)

In order to be able to enhance the Logger facility with custom classes, we need a way to handle the preferred file opening modes of those classes. I enhanced the current log writer and reader classes so that each class can specify an own preferred file opening mode which is respected by Logger and LogReader when handling gzip-compressed files.

Additionally, I fixed a small typo leading to several test failures.

Please review and thank you very much for your work!

@zariiii9003
Copy link
Collaborator

Could you explain, which problem you are trying to solve with the PREFERRED_MODE class attribute?

@ro-i
Copy link
Contributor Author

ro-i commented May 8, 2023

Sure! I wanted to create own subclasses of MessageWriter/MessageReader. They need to open the underlying files in binary mode. However, the Logger and LogReader classes had hardcoded the file modes to open gzipped files. For example in Logger.compress:

         if kwargs.get("append", False):
-            mode = "ab" if real_suffix == ".blf" else "at"
+            mode = LoggerType.PREFERRED_MODE_APPEND
         else:
-            mode = "wb" if real_suffix == ".blf" else "wt"
+            mode = LoggerType.PREFERRED_MODE

Previously, it was hardcoded that only files for the .blf suffix would be opened in binary mode. Now, every MessageReader/MessageWriter can specify its own preferred file opening mode.

@zariiii9003
Copy link
Collaborator

I understand now. But we just need to know, whether the log format is binary or not, right?

What do you think about adding these new types to generic.py:

class TextIOMessageWriter(FileIOMessageWriter, metaclass=ABCMeta):
    file: typing.TextIO


class BinaryIOMessageWriter(FileIOMessageWriter, metaclass=ABCMeta):
    file: typing.BinaryIO | gzip.GzipFile

Then the gzip-mode could just be dependent on a subclass check:

        if issubclass(logger_type, BinaryIOMessageWriter):
            mode = "ab" if append else "wb"
        else:
            mode = "at" if append else "wt"

@ro-i
Copy link
Contributor Author

ro-i commented May 15, 2023

Thanks for the feedback! I adapted the code (and also considered the *Reader class). Did you have something like that in mind?

@@ -105,5 +108,21 @@ def file_size(self) -> int:
return self.file.tell()


class TextIOMessageWriter(FileIOMessageWriter, metaclass=ABCMeta):
file: TextIO | gzip.GzipFile
Copy link
Collaborator

Choose a reason for hiding this comment

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

GzipFile returns a TextIOWrapper when opened in text mode, so this should be TextIO only

@ro-i ro-i force-pushed the file-mode branch 2 times, most recently from eea1e8d to e083462 Compare May 15, 2023 15:11
Copy link
Collaborator

@zariiii9003 zariiii9003 left a comment

Choose a reason for hiding this comment

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

🚀 Thank you for your contribution

@zariiii9003 zariiii9003 merged commit b61482a into hardbyte:develop May 16, 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