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

Set default value of filterset data to MultiValueDict #1634

Merged
merged 2 commits into from Mar 8, 2024

Conversation

sgordon16
Copy link
Contributor

fix for issue #1633

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (af9f13b) 98.59% compared to head (97ec657) 98.59%.
Report is 1 commits behind head on main.

❗ Current head 97ec657 differs from pull request most recent head 390c0f2. Consider uploading reports for the commit 390c0f2 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1634   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files          15       15           
  Lines        1283     1284    +1     
=======================================
+ Hits         1265     1266    +1     
  Misses         18       18           

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

Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @sgordon16 — just to let you know, I'm not ignoring this.

I think it looks great. (I can't think of any issues, unless folks are doing weird isinstance checks...)

I'm slowly edging towards a release, and this will be part of that.

@carltongibson
Copy link
Owner

If you had capacity though, adding a test that demonstrates the change in behaviour would be handy. Thanks.

@sgordon16
Copy link
Contributor Author

If you had capacity though, adding a test that demonstrates the change in behaviour would be handy. Thanks.

Done

@sgordon16
Copy link
Contributor Author

I also just want to point out that this is what Django does in the BaseForm class here.

@carltongibson
Copy link
Owner

@sgordon16 Yes, it's certainly correct. I would have merged it already, but I want to play with a couple of things in the shell, and just haven't had time for that yet.

🎁

Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, brilliant. Let's have it. Thanks @sgordon16 🎁

@carltongibson carltongibson merged commit 296cabe into carltongibson:main Mar 8, 2024
1 check passed
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.

None yet

3 participants