Skip to content

feat: handle request.GET or request.POST being None + a few other improvements @W-18128971 #173

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

Merged
merged 2 commits into from
Apr 8, 2025

Conversation

snacu
Copy link
Collaborator

@snacu snacu commented Apr 3, 2025

Work Item: W-18078839

  • Add a few docstring and inline comments to modified methods
  • Improve boolean type handling with more valid values
  • Improve error handling with detailed messages and logging
  • Add explicit checks for None request and query_dict
  • Improve field name handling with explicit checks
  • add unit tests for RequstField.get_without_default method

  • Update CHANGELOG.md
  • Update README.md (as needed)
  • New dependencies were added to pyproject.toml
  • pip install succeeds with a clean virtualenv
  • There are new or modified tests that cover changes
  • Test coverage is maintained or expanded

@snacu snacu requested a review from lukka5 April 3, 2025 00:32
field = machinery.field(type=str)
actual = field.get_without_default(None, request)
self.assertIsNone(actual)

Copy link
Collaborator

Choose a reason for hiding this comment

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

we could maybe add a test with request with query_dict but without name or api_name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that would fail in a different way because the there's a meta class that has to fill in that name and in the tests it would need to be mocked

int_field.get_without_default(None, request)

self.assertIn(
f"Could not parse {antagonistical_value} as type int", str(cm.exception)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could also check for the presence of new log ev=dda, loc=coerce_value_to_type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see much benefit from asserting the log line is there, also makes the logging updates difficult after (tests would need update too)

Copy link
Collaborator Author

@snacu snacu left a comment

Choose a reason for hiding this comment

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

Thanks!

field = machinery.field(type=str)
actual = field.get_without_default(None, request)
self.assertIsNone(actual)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that would fail in a different way because the there's a meta class that has to fill in that name and in the tests it would need to be mocked

int_field.get_without_default(None, request)

self.assertIn(
f"Could not parse {antagonistical_value} as type int", str(cm.exception)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see much benefit from asserting the log line is there, also makes the logging updates difficult after (tests would need update too)

@snacu snacu merged commit 1f2a845 into main Apr 8, 2025
5 checks passed
@snacu snacu deleted the handle-post-body-none branch April 8, 2025 22:17
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