Skip to content

Conversation

@RobertoRoos
Copy link
Contributor

@RobertoRoos RobertoRoos commented Oct 5, 2021

Changes:

  • Moved Connection class to it's own file
  • Split pyads_ex into ads...() functions and support methods
  • Made type casting a little more dry with new function
  • Cleaned up SumRead/Write a little bit

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
Copy link

Pull Request Test Coverage Report for Build 1308310190

  • 268 of 271 (98.89%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 94.691%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyads/connection.py 234 237 98.73%
Totals Coverage Status
Change from base Build 1207035762: 0.3%
Covered Lines: 1623
Relevant Lines: 1714

💛 - Coveralls

@stlehmann
Copy link
Owner

@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.

@stlehmann stlehmann merged commit 1d557d4 into stlehmann:master Oct 15, 2021
@RobertoRoos
Copy link
Contributor Author

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.

@RobertoRoos RobertoRoos deleted the feature/refactor branch October 19, 2021 08:03
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.

3 participants