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

feat: add Exception type to public api #246

Merged
merged 10 commits into from
Jan 21, 2024

Conversation

creberust
Copy link
Contributor

Hi,

I've added the Exception type to the public API in order to be able to use it in the Service implementation first.

I've also modified the examples to show how to return an Exception from the Service, it requires to change the Service trait. Thus, if it's too much for this PR, I can split it up and keep this PR very short.

This is a duplicate of #218 but the author seems unavailable.

@creberust creberust force-pushed the add-exception-to-public-api branch from c8a51d0 to eee13e1 Compare January 16, 2024 09:36
@creberust creberust marked this pull request as ready for review January 16, 2024 09:36
@uklotzde
Copy link
Member

Thanks. All changes look reasonable. Does this PR resolve #169?

Would you mind adding a change log entry? We need to bump the version to v0.11.0 even though client-only code is unaffected. Not a big deal.

What's the reason for using two different GitHub accounts? I was confused and wondering why GitHub requires approval to run the CI builds again.

@uklotzde uklotzde added this to the v0.11.0 milestone Jan 16, 2024
@creberust
Copy link
Contributor Author

Hi,

this resolves only the Server side Exception handling of the #169 . But I think it's the easiest minimal change to keep the current code working without modifying too much, and yet adding simplicity to Exception handling in the Server side.

In order to resolve #169, we need to also update the Client trait to better handle return to the user, the correct Exception. I think this will be harder because we need to modify a lot of trait, and some of the logic with the Client handling of errors.

I'll add a changelog entry 👍🏻 .

Oh sorry for the confusion, this one is my personal account and the other one is my work account. I'll only be using this one in the future, sorry for the unexpected change.

@creberust
Copy link
Contributor Author

I can also add tests to be sure that the FunctionCode for each Response in case of an Exception is the correct one.

@creberust
Copy link
Contributor Author

creberust commented Jan 19, 2024

Hello 👋🏻

I've added a change log entry for each of the changes.

I've also added integration tests for tcp-server, rtu-server and rtu-over-tcp-server in order to check that Exception's are correctly returned.

But I've run into 2 issues:

  • Integration test for rtu-server can't be run because we need an available SerialPort. We need to mock one or change the way to test.
  • I've found a hang when using Request::Custom with rtu-over-tcp-server. I've commented the code where the mistakes happens. It only hangs here with feature rtu-over-tcp-server.

We'll need to update the tests when the client will handle the Exception.

@creberust
Copy link
Contributor Author

creberust commented Jan 19, 2024

Oh I forgot to add pre-commit 😅 will do it now and commit the fix for the errors.

Copy link
Member

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you for your efforts!

@uklotzde uklotzde merged commit b481976 into slowtec:main Jan 21, 2024
10 checks passed
@creberust creberust deleted the add-exception-to-public-api branch January 22, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants