Skip to content

Support nullable ListFields with django-rest-framework#78

Merged
browniebroke merged 4 commits intodjango-commons:mainfrom
Rmvandiepen:patch-1
Oct 11, 2024
Merged

Support nullable ListFields with django-rest-framework#78
browniebroke merged 4 commits intodjango-commons:mainfrom
Rmvandiepen:patch-1

Conversation

@Rmvandiepen
Copy link
Copy Markdown
Contributor

@Rmvandiepen Rmvandiepen commented Oct 4, 2023

As mentioned in #61 it doesn't work when a ListField has a null value in it.
This should fix that 🥳

@FlipperPA
Copy link
Copy Markdown
Member

FlipperPA commented Oct 10, 2023

@Rmvandiepen Hey, I very much appreciate the PR! I'm hoping to take some time at DjangoCon US this week during sprints to catch up on merges. Didn't want you to think I was ignoring you!

@FlipperPA
Copy link
Copy Markdown
Member

FlipperPA commented Jun 22, 2024

@rptmat57 @melvinkcx This is a PR for NULL fields. I'm pretty sure we still need to merge this one to cover all edge cases, but wanted to run it by both of you since you encountered and fixed the edge case from yesterday. Thoughts?

melvinkcx
melvinkcx approved these changes Jun 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@melvinkcx melvinkcx left a comment

Choose a reason for hiding this comment

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

LGTM!

@rptmat57
Copy link
Copy Markdown
Contributor

rptmat57 commented Jun 22, 2024

same, looks good!

@browniebroke
Merge branch 'main' into patch-1
9a0f47f 
# Conflicts:
#	drf_excel/fields.py
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 11, 2024 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@339e088). Learn more about missing BASE report.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main      #78   +/-   ##
=======================================
  Coverage        ?   92.87%           
=======================================
  Files           ?        4           
  Lines           ?      351           
  Branches        ?        0           
=======================================
  Hits            ?      326           
  Misses          ?       25           
  Partials        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

browniebroke
browniebroke approved these changes Oct 11, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

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

I've merged in latest chanegs from the main branch and expended the new test suite to cover this extra case, which made me realised that the empty list case wasn't covered

@browniebroke browniebroke merged commit aa02c51 into django-commons:main Oct 11, 2024
@browniebroke
Copy link
Copy Markdown
Member

browniebroke commented Oct 11, 2024

Thanks a lot for your contribution and your patience 🎉

@browniebroke browniebroke mentioned this pull request Oct 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@browniebroke browniebroke browniebroke approved these changes

+1 more reviewer

@melvinkcx melvinkcx melvinkcx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Rmvandiepen @FlipperPA @rptmat57 @codecov-commenter @browniebroke @melvinkcx