stlehmann / pyads

Python wrapper for TwinCAT ADS
MIT License
254 stars 94 forks source link

Refactoring of pyads_ex in preparation for symbol list feature #269

Closed RobertoRoos closed 2 years ago

RobertoRoos commented 2 years ago

Changes:

Contains the general refactoring originally from #268 . This PR should be merged before that other one.

The reason for the new files is the old files were too long. Arguably they still are, but it's a bit better now.

Hm, codefactor.io is not very impressed: https://www.codefactor.io/repository/github/robertoroos/pyads/overview/feature/refactor

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 1308310190


Changes Missing Coverage Covered Lines Changed/Added Lines %
pyads/connection.py 234 237 98.73%
<!-- Total: 268 271 98.89% -->
Totals Coverage Status
Change from base Build 1207035762: 0.3%
Covered Lines: 1623
Relevant Lines: 1714

💛 - Coveralls
stlehmann commented 2 years ago

@RobertoRoos great work. Putting the Connection class in its own module was long due. The implementation of the additional support functions also looks good for me. Even the coverage increased. As we have 3 additional functions I think we should add specific unit tests for them even though they are covered by the existing tests. I will create a new issue for this and kindly ask you to take care of it.

RobertoRoos commented 2 years ago

Thanks. Yes, sounds like a good idea. I am not entirely satisfied with the split of adsSum...Bytes, there might be a better method. I'll rebase the new symbol list read/write on top of this, work out a good distribution of logic and make commits to the right branches.