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

Refact echoscp #87

Merged
merged 8 commits into from
Dec 15, 2024
Merged

Refact echoscp #87

merged 8 commits into from
Dec 15, 2024

Conversation

dwikler
Copy link
Collaborator

@dwikler dwikler commented Dec 15, 2024

This merge request addresses part of issue #85.
It refactors echoscp.py from the tdwii_plus_examples/TDWII_PPVS_subscriber folder.
The EchoSCP class is refactored as CEchoSCP and moved to tdwii_plus_examples package folder in cechoscp.py
The main function is refactored as an echoscp.py CLI application and moved to the tdwii_plus_examples/cli folder.

Copy link
Owner

@sjswerdloff sjswerdloff left a comment

Choose a reason for hiding this comment

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

Please change class TestBaseSCP in test_echoscp.py to TestCEchoSCP

tdwii_plus_examples/tests/test_cechoscp.py Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

do we want to add a test of the cli version in the future?
The underlying class is getting tested, so I don't think it's critical unless the cli version is intended to get significant use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will have to do it for the next steps cli apps so I will add it at the same time.

Change test class to TestCEchoSCP
@sjswerdloff sjswerdloff merged commit d46e645 into sjswerdloff:main Dec 15, 2024
1 check passed
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