-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: enhance file module with improved documentation and tests #92
base: main
Are you sure you want to change the base?
feat: enhance file module with improved documentation and tests #92
Conversation
- Add type validation for template body fields - Implement binary record support with length validation - Add port number validation (1-100) - Support compact format with metadata field validation - Update API documentation with new features - Add comprehensive test suite Co-Authored-By: [email protected] <[email protected]>
Co-Authored-By: [email protected] <[email protected]>
Co-Authored-By: [email protected] <[email protected]>
…ement Co-Authored-By: [email protected] <[email protected]>
…thesis Co-Authored-By: [email protected] <[email protected]>
Co-Authored-By: [email protected] <[email protected]>
- Add file parameter to file.stats method - Update docstrings with accurate return type information - Add comprehensive test coverage for file module methods - Split tests into separate files for better organization Co-Authored-By: [email protected] <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
⚙️ Control Options:
|
docs/api.md
Outdated
* `file` The name of the file. | ||
* `body` A JSON object to add to the note. | ||
* `payload` An optional base64-encoded string. | ||
* `binary` Binary data (bytearray) to be stored in the note. |
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.
binary is a boolean value, not a byte array. please see these docs: https://dev.blues.io/api-reference/notecard-api/note-requests/latest/#note-add
notecard/file.py
Outdated
response = card.Transaction(req) | ||
if "err" in response: | ||
return response | ||
# Check for required fields first |
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.
checking for "required" fields in the response is not a valid test. this is because notecard uses omitempty by default, meaning if there is a 0, false, or empty string "", that value would be omitted from the response.
notecard/file.py
Outdated
response = card.Transaction(req) | ||
if "err" in response: | ||
return response | ||
# Check for required fields |
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.
checking for "required" fields in the response is not a valid test. this is because notecard uses omitempty by default, meaning if there is a 0, false, or empty string "", that value would be omitted from the response.
notecard/file.py
Outdated
response = card.Transaction(req) | ||
if "err" in response: | ||
return response | ||
# Validate response format - should contain total and changes |
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.
checking for "required" fields in the response is not a valid test. this is because notecard uses omitempty by default, meaning if there is a 0, false, or empty string "", that value would be omitted from the response.
notecard/note.py
Outdated
return {"err": "verify parameter must be a boolean"} | ||
|
||
if length is not None: | ||
if not isinstance(length, int) or length < 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.
length can be a negative number (it effectively resets the value to its default)
notecard/note.py
Outdated
req["format"] = "compact" | ||
if body: | ||
allowed_metadata = {"_time", "_lat", "_lon", "_loc"} |
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.
_loc is not a valid value here, it should be replaced with _ltime
test/fluent_api/test_file_changes.py
Outdated
} | ||
response = file.changes(card) | ||
# First validate the response has all required fields | ||
assert 'changes' in response |
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.
these assert statements are invalid since notecard uses omitempty and those values may not exist on the response
Devin is currently unreachable - the session may have died. |
File Module Improvements
This PR enhances the file module with improved documentation and test coverage to better align with the official Notecard API documentation.
Changes
Testing
Documentation
Updated docstrings to accurately reflect:
Link to Devin run: https://app.devin.ai/sessions/95ffd763d71b445198b9370e49dcd93f