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

REPL client params are broken #2006

Closed
alexrudd2 opened this issue Feb 11, 2024 · 11 comments · Fixed by #2007
Closed

REPL client params are broken #2006

alexrudd2 opened this issue Feb 11, 2024 · 11 comments · Fixed by #2007

Comments

@alexrudd2
Copy link
Collaborator

----------------------------------------------------------------------------
__________          _____             .___  __________              .__
\______   \___.__. /     \   ____   __| _/  \______   \ ____ ______ |  |
 |     ___<   |  |/  \ /  \ /  _ \ / __ |    |       _// __ \\\____ \|  |
 |    |    \___  /    Y    (  <_> ) /_/ |    |    |   \  ___/|  |_> >  |__
 |____|    / ____\____|__  /\____/\____ | /\ |____|_  /\___  >   __/|____/
           \/            \/            \/ \/        \/     \/|__|
                                        v1.3.0 - 3.7.0dev
----------------------------------------------------------------------------

> client.get_parity
'_params' object has no attribute 'parity'

I think params needs to become comm_params. PR incoming

@janiversen
Copy link
Collaborator

Looks like a bug, please let me investigate.

@janiversen
Copy link
Collaborator

and I just read the last part....yes we are moving away from .params as fast as can, and there are probably a small number that have changed recently.

Thanks for helping out on this.

@alexrudd2
Copy link
Collaborator Author

There are still not tests for REPL, and I don't know enough to write them. However, the linked PR "teaches" mypy and friends about the base classes, so it can check the REPL more strictly.

@janiversen
Copy link
Collaborator

Yeah "someone" promised to make quite a while ago, but it seems he has dropped completely out.

It is on my list, but behind a number of more urgent matters.

@dhoomakethu
Copy link
Contributor

Who is that someone 😎???. I am sorry I am unable to keep up with the changes happening both personally and with this project. The best thing would be to move REPL out as a separate repo (I believe I have this proposed in the past as well) and keep only the core library as part of pymodbus.

@janiversen
Copy link
Collaborator

Yes it has been proposed by the same person.

I think I will do it for v3.7.0, I agree it is a hazzle, mostly because the code have not been updated for a long time and thus depends on the old architecture.

@dhoomakethu heads up, in a couple of months REPL will be in each own repo....we might reintegrate it at a later date, when the simulator is finished, because it is a real nice feature to have.

@dhoomakethu
Copy link
Contributor

I have a version ready with some changes, I will push it in sometime for initial discussion.

@alexrudd2
Copy link
Collaborator Author

Yes it has been proposed by the same person.

whoosh! 😆

Who is that someone 😎???. I am sorry I am unable to keep up with the changes happening both personally and with this project.

Haha, I also find it difficult to keep up with the mad Dane. It's one of the reasons I focus on automatic tools - to let the computer do the work.

Thanks to you both, of course. I used 2.5.3 very heavily, and 3.x now also.

@janiversen
Copy link
Collaborator

I did not want to sound negative or hit on That "someone" is @dhoomakethu but I think he never got time to make test cases...please do not take this negative, we all have different priorities and

@dhoomakethu I am not I understand what you mean "happening both personally and ....", I hope you are not having a hard time !

This mad Dane (others just write "Viking", that explains it all), have a clear objective, which is to make pymodbus more robust. I really do my best to document all external changes in API_CHANGES, but like @alexrudd2 I depend heavily on CI to find places where a change have unwanted side effects, so missing tests means no red flags.

My short time plan is to expand our tests heavily (transport now have 100% coverage), simply because I too get tired of seeing regression bugs. Most of my other changes are actually down in the framer and transport.

This Viking is not alone in changing everything, we have had a constant flow of new developers making small changes, I take this as a healthy sign for the project. Lately there have been a lot of focus on type checking, and I have slowly been turned, so now all my new code are typed (big thanks to @alexrudd2 for heading this effort).

@dhoomakethu
Copy link
Contributor

@janiversen I understand and no offense taken. Thanks for the concern. I know I have in the past talked about many things and I am not at all contributing. I will start contributing back on certain parts (hopefully) starting with repl.

@janiversen
Copy link
Collaborator

Looking forward to that....you are the "old" guy in this project, with a lot more experience than I.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2024
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 a pull request may close this issue.

3 participants