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

serial: switch to serial_for_url instead of Serial #393

Merged
merged 2 commits into from
Aug 27, 2018

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