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

Add ConnectedServer Property to AprsIsClient #134

Merged

Conversation

martelro
Copy link
Contributor

Description

As described in #120, a rotating address is often used to connect to an APRS-IS server; however, the actual connected server is not known to the user.

This change adds parsing to the AprsIsClient class to obtain the connected server and expose it as a property of the class.

Changes

  • Add nullable public property, ConnectedServer, to AprsIsClient
  • Add parsing logic via private method to obtain the server name from the connected server's login response and assign to ConnectedServer. If server name cannot be determined, the property value is set to null.
  • Private method also logs server name

Validation

  • Build passes
  • Created and passed unit tests
  • Ran locally at LogInformation visibility level to confirm server name is captured in property

martelro and others added 2 commits May 29, 2022 08:38
Add SetConnectedServer method
Add test cases for SetConnectedServer

CBielstein#120
@CBielstein CBielstein linked an issue Jun 1, 2022 that may be closed by this pull request
4 tasks
@CBielstein CBielstein added the enhancement New feature or request label Jun 1, 2022
Copy link
Owner

@CBielstein CBielstein left a comment

Choose a reason for hiding this comment

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

This looks really great! Clearly you know your way around the C# stack more than the descriptions gave you credit for. No offense intended there, just never know the experience level and want to be welcoming to newbies as well. 😊

Just caught the one race condition, which I only caught as my PC for some reason fails on it 100% of the time. I wouldn't have seen it otherwise. There are also a few line of test code that came over from the other test but aren't necessary here that we can clean up.

Overall, looks fantastic (especially in a bit of a messy code base like ours 😁)! Please make the little tweaks and I'm excited to merge this in!

test/AprsIsClientUnitTests/ReceiveUnitTests.cs Outdated Show resolved Hide resolved
src/AprsIsClient/AprsIsClient.cs Show resolved Hide resolved
src/AprsIsClient/AprsIsClient.cs Show resolved Hide resolved
…ivedUnitTests

Place SetConnectedServer before state change in AprsIsClient class Receive method

CBielstein#120
@martelro
Copy link
Contributor Author

martelro commented Jun 1, 2022

Thank you for the kind review. Admittedly, I know some areas of C# better than others, but those will come with time and practice I hope!

I've pushed the updates, but please let me know if there's anything else. Thank you!

Copy link
Owner

@CBielstein CBielstein left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks for the updates and the code in general! You're more than welcome to code with us whenever you'd like! 😎

@CBielstein
Copy link
Owner

Commenting for my own sake: I did rerun the build task due to test timeout failures, but these were not related to this test. They are a known issue of our parallelism and such issues would be resolved by #125. With that in mind, I'm still merging this PR and we can fix #125 asap to ensure this doesn't happen again.

@CBielstein CBielstein merged commit 5796c0e into CBielstein:main Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AprsIsConnection should save which APRS-IS server it is connected to
2 participants