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

Feature/update tag value #573

Merged

Conversation

AlcibiadesCleinias
Copy link
Contributor

@AlcibiadesCleinias AlcibiadesCleinias commented Jul 23, 2021

Hi,

there was a discussion with possible implementation: #498.
Then, @ldariva contribute with pr: https://github.com/Hugovdberg/PIconnect/pull/540/files but it still is opened and with some changes suggested.

  • so, my pr fully relies on the pr mentioned above
  • In a code, I allowed myself to leave some function argument annotations. I believe with annotations it is easier to understand what should be passed to a method and IDE shows hints, and pep8... (until the last I have tried to follow the main rule of pep8: cohesive module style, but I guess with such annotations it is rly more comfortable and readable)
  • with the new update method, it is possible to change old values with the right combination of the method options
  • rm todo in the readme and wrote features list instead (not sure if completed)

Acknowledgment

PS.
It is better to review and merge after #572

@AlcibiadesCleinias
Copy link
Contributor Author

M-m, @Hugovdberg I saw your use of int(IntEnum). Guess, I will change behaviour to yours, i.e. instead of IntEnum.value -> int(IntEnum). So leave the possibility to pass to a method integers

@Hugovdberg
Copy link
Owner

I chose this implementation to allow for direct use of int values indeed, which I believe was mostly to accommodate for versions of python.

Copy link
Owner

@Hugovdberg Hugovdberg left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution. This is a valuable addition to the package, as it was a widely requested feature. I have added a few comments for things I would like to see slightly different before merging. Also, don't forget to add yourself to the AUTHORS.rst as a contributor!

@Hugovdberg Hugovdberg merged commit 0ffae8f into Hugovdberg:develop Jul 28, 2021
@Hugovdberg Hugovdberg mentioned this pull request Jul 28, 2021
@Hugovdberg Hugovdberg mentioned this pull request Aug 10, 2021
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.

2 participants