Skip to content

serial: switch to serial_for_url instead of Serial#393

Merged
felixdivo merged 2 commits intohardbyte:developfrom
yegorich:url_to_serial
Aug 27, 2018
Merged

serial: switch to serial_for_url instead of Serial#393
felixdivo merged 2 commits intohardbyte:developfrom
yegorich:url_to_serial

Conversation

@yegorich
Copy link
Contributor

This way one can also use remote ports via socket:// or rfc2217:// URLs.

@codecov
Copy link

codecov bot commented Aug 22, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@80ca49f). Click here to learn what that means.
The diff coverage is 50%.

@@            Coverage Diff             @@
##             develop     #393   +/-   ##
==========================================
  Coverage           ?   59.27%           
==========================================
  Files              ?       55           
  Lines              ?     4263           
  Branches           ?        0           
==========================================
  Hits               ?     2527           
  Misses             ?     1736           
  Partials           ?        0

@felixdivo
Copy link
Collaborator

Hey, looks cool! Could you document that feature somewhere? Is is possible to test this?

@yegorich
Copy link
Contributor Author

v3 with documentation. As for the tests one can probably use loop:// URL.

@felixdivo
Copy link
Collaborator

Looks good! Could you also add some tests, maybe by extending the existing ones to also work with loop://? Leaving a note in the docs about the loop:// option might benefit others as well.

@yegorich
Copy link
Contributor Author

@felixdivo I've made a test with loop:// and it is working so far. Should I create an extra file like test/serial_test_loop.py or should this just replace test/serial_test.py? The loop based test doesn't need mock.

As for documentation I think mentioning loop:// will only make sense in serial.rst.

@felixdivo
Copy link
Collaborator

Would it make sense to run SimpleSerialTest twice, on time with mock and the other time with loop://`?

You are right about the docs.

This way one can also use remote ports via socket:// or rfc2217:// URLs.
@yegorich
Copy link
Contributor Author

v4: can/interfaces/serial/serial_can.py performs tests with both mock and loop:// approaches. Mention loop:// in doc/interfaces/serial.rst.

@felixdivo
Copy link
Collaborator

That looks great to me! Let's just wait for the CI tests to run once more and then I'll merge it.

@felixdivo felixdivo merged commit c034773 into hardbyte:develop Aug 27, 2018
@yegorich yegorich deleted the url_to_serial branch August 27, 2018 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants