IO module enhancements #348

felixdivo · 2018-07-03T15:37:16Z

This PR harmonizes the different CAN message writers and readers, fixes some bugs and adds new features.

Features

context manager support for all readers and writers
they share a common super class called BaseIOHandler
all file handles can now be closed with the stop() method
the table name in SqliteReader/SqliteWriter can be adjusted
append mode added in CSVWriter and CanutilsLogWriter
file-like and path-like objects can now be passed to the readers and writers (except to the Sqlite handlers)
add a __ne__() method to the Message class (this was required by the tests)
added a stop() method for BufferedReader
SqliteWriter: 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 database

Breaking changes

writing to closed writers is not supported any more (it was supported only for some)
the method Listener.on_message_received() is now abstract (using @abc.abstractmethod)
the file in the reader/writer is now always stored in the attribute uniformly called file, and not in something like fp, log_file or output_file
removed the unused header_line attribute from CSVReader
privatized some attributes that are only to be used internally in the classes
changed the name of the first parameter of the read/writer constructors from filename to file

Non-breaking changes

added documentation
removed a lot of noisy debugging statements
added quite a few tests

Fixes

the __iter__() mthods now call stop() at the end of the iteration
turned CSVReader and SqliteReader into new-style classes
added CSVReader and SqliteReader to docs
code/doc cleanups
there are now no leftover files after running the tests
SqliteReader.read_all() now returns the messages as can.Message objects and not as raw tuples
fix a bug in ASCWriter that created corrupted files

Fixes #268.

felixdivo · 2018-07-03T15:41:59Z

@christiansandberg Do you know what's up with this failing test? It occurs on Python 2.7 only, but on both Linux & Win.

codecov · 2018-07-03T15:46:56Z

Codecov Report

Merging #348 into develop will increase coverage by 0.45%.
The diff coverage is 96.03%.

@@            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

christiansandberg · 2018-07-03T18:24:24Z

Why have you changed to use universal newlines which is deprecated and especially on binary files?

christiansandberg

Try not to change things if you don't know what you are changing. ;)

christiansandberg · 2018-07-03T19:04:38Z

can/io/generic.py

+        """
+        super(BaseIOHandler, self).__init__() # for multiple inheritance
+        if open_file:
+            self.file = open(filename, mode)


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.

Yeah, I thought about that as well. I might add it here.

christiansandberg · 2018-07-03T19:06:29Z

can/io/csv.py

-                is_error_frame=(error == '1'),
+                is_remote_frame=bool(remote),
+                extended_id=bool(extended),
+                is_error_frame=bool(error),


All non-empty strings are True.

felixdivo · 2018-07-03T20:26:52Z

I did not know that setting the universal newlines parameter with U was deprecated.

christiansandberg · 2018-07-29T18:03:19Z

can/io/logger.py

+            elif filename.endswith(".db"):
+                return SqliteWriter(filename, *args, **kwargs)
+            elif filename.endswith(".log"):
+                return CanutilsLogWriter(filename, *args, **kwargs)


Should be an else here.

No, actually not, since if none of the file types matches, it should fall back to using the Printer.

Yeah sorry. I missed that.

No problem.

christiansandberg · 2018-07-29T18:09:04Z

can/listener.py

+            if the reader has already been stopped
+        """
+        if self.is_stopped:
+            raise BufferError("reader has already been stopped")


Not sure BufferError is appropriate here. Maybe just RuntimeError or something.

exception BufferError: Raised when a buffer related operation cannot be performed.

It sort of fits, but RuntimeError does as well. I'll use RuntimeError.

christiansandberg · 2018-07-29T18:10:22Z

can/message.py

+                self.bitrate_switch == other.bitrate_switch
+            )
+        else:
+            raise NotImplementedError()


Shouldn’t it be just False?

A rich comparison method may return the singleton NotImplemented if 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?

Still, we should return the singleton and not raise the Error if we decide to leave it like this.

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.

Makes sense!

felixdivo · 2018-07-31T22:18:41Z

I want to wait for #380 before merging.

felixdivo · 2018-07-31T22:20:41Z

@christiansandberg what about #348 (comment) ? Do you have an opinion?

felixdivo · 2018-07-31T22:31:22Z

Waiting for appveyor/ci#2547 to solve the permissions problem ...

christiansandberg · 2018-08-01T06:38:37Z

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.

…-file-io

felixdivo · 2018-08-01T07:55:41Z

Ah I see, that feature was only meant for free text events. I'll adjust the tests accordingly to allow this type of behavior.

felixdivo · 2018-08-01T08:19:29Z

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.

felixdivo · 2018-08-01T17:23:58Z

Waiting for appveyor/ci#2547 to solve the permissions problem ...

This is now solved.

felixdivo added 16 commits July 3, 2018 09:38

added a generic file handler

8b18407

better docs in listener.py and made the Listener class abstract

deee1b6

make CSV reader & writer is the new BaseIOHandler

3cf6368

make Canutils reader & writer is the new BaseIOHandler

ebb36fb

docs & fixes

8798460

make ASC ahnders use BaseIOHandler

10ec6bf

fix universal nelines support

a601391

fix sphinx error

c40ab53

make stdout use BaseIOHandler

190653b

docs

3368c24

make SQlite reader/writer use the new BaseIOHandler

4d97194

added BaseIOHandler in BLF reder/Writer

906cfb1

sqlite fix

d68c430

fix file opening in BLF handlers

b10080e

rome "text mode" parameter form file open calls

0fcbf07

fix bad test cases

6d97d20

felixdivo added enhancement api docs work-in-progress labels Jul 3, 2018

felixdivo added this to the 2.3 Release milestone Jul 3, 2018

felixdivo self-assigned this Jul 3, 2018

felixdivo requested review from hardbyte and christiansandberg July 3, 2018 15:37

only writer header if not appending

9e88d10

christiansandberg requested changes Jul 3, 2018

View reviewed changes

christiansandberg reviewed Jul 29, 2018

View reviewed changes

felixdivo and others added 10 commits July 30, 2018 11:53

use RuntimeError on read attempt on a stopped BufferedReader

31734d0

Merge branch 'develop' into fix-file-io

338449f

add writable tempdir to appveyor tests

bf91364

use memoryview = buffer in Sqlite in Python 2

5449ac4

Merge branch 'develop' into fix-file-io

517ca2a

fix Message's __eq__() and __ne__()

3beb5b5

fix a bug in Python 3.7

201bf22

new attempt at making the TEMPDIR writable on AppVeyor

39eaa66

create test folder in AppVeyor

53c7ec9

also give permissions to future temp files & clean up afterwards

3bf5a6a

felixdivo mentioned this pull request Jul 31, 2018

ASC log header has duplicate lines #379

Closed

fix #379

767838b

Merge branch 'develop' into fix-file-io

2c81ec6

felixdivo added 2 commits August 1, 2018 09:55

fix wrong timestamps problem in tests

34a640b

Merge branch 'fix-file-io' of github.com:hardbyte/python-can into fix…

fa771a8

…-file-io

felixdivo added 2 commits August 1, 2018 19:15

fix apparent permissions problem on windows

d1b0503

add removal of temporyry file in listener_test.py

7be909d

felixdivo and others added 3 commits August 1, 2018 19:26

removed debugging code from .appveyor.yml

23a9d9a

only delete some temp files

a839a0a

Merge branch 'develop' into fix-file-io

a161684

felixdivo merged commit b749572 into develop Aug 6, 2018

felixdivo deleted the fix-file-io branch August 6, 2018 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IO module enhancements #348

IO module enhancements #348

felixdivo commented Jul 3, 2018 •

edited

Loading

felixdivo commented Jul 3, 2018 •

edited

Loading

codecov bot commented Jul 3, 2018 •

edited

Loading

christiansandberg commented Jul 3, 2018

christiansandberg left a comment

christiansandberg Jul 3, 2018

felixdivo Jul 3, 2018

felixdivo Jul 5, 2018

christiansandberg Jul 3, 2018

felixdivo commented Jul 3, 2018

christiansandberg Jul 29, 2018

felixdivo Jul 30, 2018

christiansandberg Jul 30, 2018

felixdivo Jul 30, 2018

christiansandberg Jul 29, 2018

felixdivo Jul 30, 2018

christiansandberg Jul 29, 2018

felixdivo Jul 30, 2018 •

edited

Loading

felixdivo Jul 30, 2018

felixdivo Jul 31, 2018

christiansandberg Jul 31, 2018

felixdivo commented Jul 31, 2018

felixdivo commented Jul 31, 2018

felixdivo commented Jul 31, 2018

christiansandberg commented Aug 1, 2018

felixdivo commented Aug 1, 2018

felixdivo commented Aug 1, 2018

felixdivo commented Aug 1, 2018

IO module enhancements #348

IO module enhancements #348

Conversation

felixdivo commented Jul 3, 2018 • edited Loading

felixdivo commented Jul 3, 2018 • edited Loading

codecov bot commented Jul 3, 2018 • edited Loading

Codecov Report

christiansandberg commented Jul 3, 2018

christiansandberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdivo commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdivo Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdivo commented Jul 31, 2018

felixdivo commented Jul 31, 2018

felixdivo commented Jul 31, 2018

christiansandberg commented Aug 1, 2018

felixdivo commented Aug 1, 2018

felixdivo commented Aug 1, 2018

felixdivo commented Aug 1, 2018

felixdivo commented Jul 3, 2018 •

edited

Loading

felixdivo commented Jul 3, 2018 •

edited

Loading

codecov bot commented Jul 3, 2018 •

edited

Loading

felixdivo Jul 30, 2018 •

edited

Loading