Conversation
|
@christiansandberg Do you know what's up with this failing test? It occurs on Python 2.7 only, but on both Linux & Win. |
Codecov Report
@@ Coverage Diff @@
## develop #348 +/- ##
==========================================
+ Coverage 58.85% 59.3% +0.45%
==========================================
Files 54 55 +1
Lines 4214 4241 +27
==========================================
+ Hits 2480 2515 +35
+ Misses 1734 1726 -8 |
|
Why have you changed to use universal newlines which is deprecated and especially on binary files? |
There was a problem hiding this comment.
Try not to change things if you don't know what you are changing. ;)
can/io/generic.py
Outdated
| """ | ||
| super(BaseIOHandler, self).__init__() # for multiple inheritance | ||
| if open_file: | ||
| self.file = open(filename, mode) |
There was a problem hiding this comment.
Would be nice if it could accept "filename" being an already open file object as well. That could open up possibilities to use other streams like over the network or compressing files on the fly.
There was a problem hiding this comment.
Yeah, I thought about that as well. I might add it here.
can/io/csv.py
Outdated
| is_error_frame=(error == '1'), | ||
| is_remote_frame=bool(remote), | ||
| extended_id=bool(extended), | ||
| is_error_frame=bool(error), |
There was a problem hiding this comment.
All non-empty strings are True.
|
I did not know that setting the universal newlines parameter with |
| elif filename.endswith(".db"): | ||
| return SqliteWriter(filename, *args, **kwargs) | ||
| elif filename.endswith(".log"): | ||
| return CanutilsLogWriter(filename, *args, **kwargs) |
There was a problem hiding this comment.
Should be an else here.
There was a problem hiding this comment.
No, actually not, since if none of the file types matches, it should fall back to using the Printer.
There was a problem hiding this comment.
Yeah sorry. I missed that.
can/listener.py
Outdated
| if the reader has already been stopped | ||
| """ | ||
| if self.is_stopped: | ||
| raise BufferError("reader has already been stopped") |
There was a problem hiding this comment.
Not sure BufferError is appropriate here. Maybe just RuntimeError or something.
There was a problem hiding this comment.
exception BufferError: Raised when a buffer related operation cannot be performed.
It sort of fits, but RuntimeError does as well. I'll use RuntimeError.
can/message.py
Outdated
| self.bitrate_switch == other.bitrate_switch | ||
| ) | ||
| else: | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
Shouldn’t it be just False?
There was a problem hiding this comment.
A rich comparison method may return the singleton
NotImplementedif it does not implement the operation for a given pair of arguments.
Do we explicitly not support it or is it just False? Maybe, the other object would be able to compare a message to them?
There was a problem hiding this comment.
Still, we should return the singleton and not raise the Error if we decide to leave it like this.
There was a problem hiding this comment.
According to this, when we return NotImplemented, the other object "b" is asked if it can compare itself to the message. I guess that's desirable, or at least not harming.
|
I want to wait for #380 before merging. |
|
@christiansandberg what about #348 (comment) ? Do you have an opinion? |
|
Waiting for appveyor/ci#2547 to solve the permissions problem ... |
|
I added that functionality to blf and asc formats as you can log free text events apart from CAN messages, and since you shouldn’t have entries with a timestamp from 1970 it made sense to pick something valid. This format can only log CAN messages and they could be assumed to always have a timestamp so in this case I don’t think it matters. |
|
Ah I see, that feature was only meant for free text events. I'll adjust the tests accordingly to allow this type of behavior. |
|
Apart from the permissions problem mentioned above and the problem in #380, this now seems to work. We should wait for both to be solved/merged before merging this PR into develop, just to be sure to not introduce any bugs into the develop branch with this huge PR. |
This is now solved. |
This PR harmonizes the different CAN message writers and readers, fixes some bugs and adds new features.
Features
BaseIOHandlerstop()methodSqliteReader/SqliteWritercan be adjustedCSVWriterandCanutilsLogWriter__ne__()method to theMessageclass (this was required by the tests)stop()method forBufferedReaderSqliteWriter: this now guarantees that all messages are being written, exposes some previously internal metrics and only buffers messages up to a certain limit before writing/committing to the databaseBreaking changes
Listener.on_message_received()is now abstract (using@abc.abstractmethod)file, and not in something likefp,log_fileoroutput_fileheader_lineattribute fromCSVReaderfilenametofileNon-breaking changes
Fixes
__iter__()mthods now callstop()at the end of the iterationCSVReaderandSqliteReaderinto new-style classesCSVReaderandSqliteReaderto docsSqliteReader.read_all()now returns the messages ascan.Messageobjects and not as raw tuplesASCWriterthat created corrupted filesFixes #268.