stlehmann / pyads

Python wrapper for TwinCAT ADS
MIT License
252 stars 93 forks source link

Move symbols to separate package pyads-symbols #298

Open stlehmann opened 2 years ago

stlehmann commented 2 years ago

Symbols are a nice object-oriented way to handle variables in pyads. Alghough I personally haven' t used them, yet I think it is a very convenient abstraction layer and should be elaborated further in the future.

At the moment symbols are some kind of concurrent approach to the normal Connection functions like read_by_name, etc. This leads to redundant code and more importantly much more code which is hard to maintain. Also we have seen that adding additional features like a list read/write function for symbols can lead to heavy refactoring (#268).

As the main focus of this repository is to provide a robust wrapper for the adslib it is my wish to move symbols in a separate package (which could be called pyads-symbols). This package should use pyads as a backbone and build the nice object-oriented symbol handling on top. This way we ensure the robustness of pyads while putting an abstraction layer on top which can grow independently without adding complexity to pyads.

stlehmann commented 2 years ago

@RobertoRoos, @chrisbeardy could you imagine going this way?

RobertoRoos commented 2 years ago

Kind of, yes. Though I would have the find the courage to set that up :')

Out of curiosity, would you fork pyads or create a dependent package? Problem when not forking is some code will still not be DRY, as we saw during a recent pull request attempt. I.e., much pyads code will have to be copied and slightly modified.

stlehmann commented 2 years ago

@RobertoRoos sounds great.

My idea is to set pyads-symbols on top of pyads as kind of an addon. Comparing to a fork I see the main benefits of an addon in:

This way symbols can always benefit from updates and improvements of pyads directly. If you go for a heavily refactored fork I think it might be hard to merge updates from upstream (and test them, because tests might differ).

Concerning the DRY-pattern I think a pragmatic step-wise approach would be good here:

  1. transfer current symbols feature along with tests and documenation
  2. add additional features like list read/write (try to use the basic pyads functions and avoid code repetition as far as possible)
  3. improve/add some functions in pyads to provide a better API for symbols
  4. remove code-doublings of pyads-symbols
chrisbeardy commented 2 years ago

You defo have the ability to set that up and maintain it @RobertoRoos. I'm sure @stlehmann and I will also be able to help with stuff such as setting up GitHub actions, build scripts, uploading to pypi etc too if required. I'm not sure the best way of dealing with the worry of DRY, there may have to be changes to pyads to enable it, that that some APIs of pyads are exposed. There's a few ways of doing it, may have to play with a couple.

You could import pyads and then extend the Connection class and then add the Symbol classes, that way you get all the functionality of pyads Connection class built in, it then kind of means to a user they only really need to then change their import from pyads to pyads-symbols and they get the symbol functionality. That way seems like little code reuse. I think in much advanced cases the user may also import pyads to get use of exposed helper functions and constants, I see no harm there.

An other option is that a user handles the connection etc in their code and passes the connection to the pyads-symbols package somehow. This also seems clean in the sense its clear that's it's an add on for the user as they have to always import both pyads and pyads-symbols. But I do think there may be more duplicate code in this approach, and I believe you would then also need to import the DLL etc yourself. There would still be nothing stopping the pyads-symbols package importing the pyads package in this case to use the constants and helper functions etc.

A third way would be to have to create a separate connection. As in pyads-symbols having its own Connection class. This way I think would have the most code duplication and also I feel would be not as suitable from a user perspective.

Just some thoughts. Not sure what @stlehmann thinks.

chrisbeardy commented 2 years ago

I don't think forking would be right. Would be a nightmare to maintain.

stlehmann commented 2 years ago

An other option is that a user handles the connection etc in their code and passes the connection to the pyads-symbols package somehow. This also seems clean in the sense its clear that's it's an add on for the user as they have to always import both pyads and pyads-symbols.

I like that. Could be something like:

plc = pyads.Connection(...)
symbols = SymbolProxy(plc)

sym1 = symbols.get_symbol("GVL.i")
sym2 = symbols.get_symbol("GVL.s")
symbols.read_list([sym1, sym2])

...
stlehmann commented 2 years ago

You defo have the ability to set that up and maintain it @RobertoRoos. I'm sure @stlehmann and I will also be able to help with stuff such as setting up GitHub actions, build scripts, uploading to pypi etc too if required

@RobertoRoos I am convinced you can definetely do this and would be happy to provide some help when needed. 👍