-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
|
||
:return: boolean True set is successful | ||
""" | ||
if new_body is not None and len(new_body) == 0: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Also add tests 👀 |
|
||
:return: boolean True set is successful | ||
""" | ||
if new_data_header is not None: |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
Fix merge conflicts |
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.