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

Refactor REPL client to reduce complexity #1489

Merged
merged 3 commits into from
Apr 16, 2023
Merged

Refactor REPL client to reduce complexity #1489

merged 3 commits into from
Apr 16, 2023

Conversation

alexrudd2
Copy link
Collaborator

This splits up a large function (complexity 21) into smaller ones within a class. The tradeoff is more LOC. What do you think?

@dhoomakethu
Copy link
Contributor

This looks good to me on a first pass, I will spend some more time to see if everything is fine.

My original idea was not to bloat the core libraries with all these helpers/scripts but add them as separate projects which consumes pymodbus. It will be easier to maintain in that way.

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

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

LGTM, but I will wait for @dhoomakethu

@dhoomakethu I get your point, but in that case we should move the whole REPL out as a separate repo, and I am not sure that it is worth it.

@janiversen janiversen merged commit 9f66ab5 into dev Apr 16, 2023
@janiversen janiversen deleted the complexity branch April 16, 2023 15:17
@janiversen
Copy link
Collaborator

If @dhoomakethu have concerns later then we will address them.

@dhoomakethu
Copy link
Contributor

@janiversen yes, I had plans to move repl as an add-on package for pymodbus. I am not sure if it's worth the effort though.

@janiversen
Copy link
Collaborator

I think it is not worth the effort, it does in no way disturb the library.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants