Codecov Report

Merging #1221 (bfc3283) into develop (2807da1) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1221      +/-   ##
===========================================
- Coverage    65.86%   65.84%   -0.02%     
===========================================
  Files           86       86              
  Lines         8873     8868       -5     
===========================================
- Hits          5844     5839       -5     
  Misses        3029     3029              

Looks simple and good! 😄

Once this is merged, I suggest opening the first pre-release.

This does not really close #1210 yet. The same issue exists in can.io.logger. Also there should be a test, which reads and writes files with multiple dots.

This does not really close #1210 yet. The same issue exists in can.io.logger. Also there should be a test, which reads and writes files with multiple dots.

@Tbruno25 Do you have time for that too?

This does not really close #1210 yet. The same issue exists in can.io.logger. Also there should be a test, which reads and writes files with multiple dots.

@Tbruno25 Do you have time for that too?

Definitely! I can probably wrap this up this weekend.

Awesome!

As soon as that is merged, I'll update the changelog, publish a pre-release and create an announcement-issue.

This does not really close #1210 yet. The same issue exists in can.io.logger. Also there should be a test, which reads and writes files with multiple dots.

I'm not sure we need to explicitly read/write with multiple dots -- just that only the last suffix is used as a key to select a message reader/writer. I added a suffix with multiple dots to test_extension_matching which I think satisfies this, although we could make it a separate unittest as well if required.

LogReader.decompress and Logger.compress are almost identical except for the mode. It probably makes sense in the future to unify them as a single function that both classes either import/inherit.

LogReader.decompress and Logger.compress are almost identical except for the mode. It probably makes sense in the future to unify them as a single function that both classes either import/inherit.

Yeah, maybe. One could also easily add zip/... support too. For now, I think it is okay as is since it only requires three lines.

There are still some typing errors left after the cleanup of @zariiii9003 in #1224, would you mind fixing them (they were skipped to avoid merge conflicts)?

GzipASCReader and GzipASCWriter can be deleted, right?

There are still some typing errors left after the cleanup of @zariiii9003 in #1224, would you mind fixing them (they were skipped to avoid merge conflicts)?

GzipASCReader and GzipASCWriter can be deleted, right?

Yes, they can be removed.

@zariiii9003 Do you agree on merging this?

@zariiii9003 Do you agree on merging this?

I'll review it after work.

Ahh, one thing: The linter complains. Seems like this is unrelated though, so we can do this in a subsequent PR. I'll open an issue for this.

Well done! The docstring could be a little more precise though.

@felixdivo I do not agree with the typehints here but i will follow up with another PR that takes care of the IO type annotations in general. There are some inconsistencies that are masked by casting.

I'm interested to see the solution regarding type hints! I don't have much experience with mypy but I couldn't seem to get it happy unless I used the very vague Any.

@Tbruno25 work is continued in #1238