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

34 add setters validators #41

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

David-Rey
Copy link
Contributor

Implmented changes required for issure 34. One thing to note is that I had trouble getting my local environment to work so this code is not tested.

Copy link
Contributor

@thomasmholder thomasmholder left a comment

Choose a reason for hiding this comment

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

This is a pretty big change, so don't worry about all the comments. As some general notes, you need to get the tests passing, and probably update the test suite to using the getters and setters, which should probably come after we pull in the test suite enhancements.

Also bump versioning, but we need to figure out a standard way to do that.

EosLib/packet/packet.py Outdated Show resolved Hide resolved

:return: boolean True set is successful
"""
if new_body is not None and len(new_body) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use the body validator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to make a function or is there already a function for this? If so I can't find it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth making one, there's good logic in validate_packet.

Also this function's logic is wrong (only sets if length 0) and it doesn't return something even though the docstring specifies that it should

EosLib/packet/packet.py Outdated Show resolved Hide resolved
EosLib/packet/packet.py Outdated Show resolved Hide resolved
EosLib/packet/packet.py Outdated Show resolved Hide resolved
EosLib/packet/packet.py Outdated Show resolved Hide resolved
EosLib/packet/data_header.py Outdated Show resolved Hide resolved
@thomasmholder thomasmholder linked an issue Feb 8, 2023 that may be closed by this pull request
@thomasmholder
Copy link
Contributor

Also add tests 👀


:return: boolean True set is successful
"""
if new_data_header is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is very incorrect

@thomasmholder thomasmholder self-requested a review February 22, 2023 00:30
Copy link
Contributor

@thomasmholder thomasmholder left a comment

Choose a reason for hiding this comment

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

Some very tiny style things, and then you need to merge the tests from main and integrate yours with those. Otherwise this is looking good.

EosLib/packet/data_header.py Outdated Show resolved Hide resolved
EosLib/packet/data_header.py Outdated Show resolved Hide resolved
EosLib/packet/packet.py Outdated Show resolved Hide resolved
EosLib/packet/transmit_header.py Outdated Show resolved Hide resolved
@thomasmholder
Copy link
Contributor

Fix merge conflicts

@David-Rey David-Rey requested a review from thomasmholder April 2, 2023 18:56
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.

Add setters and validators
2 participants