unfoldadmin / django-unfold

Modern Django admin theme for seamless interface development
https://unfoldadmin.com
MIT License
1.95k stars 188 forks source link

Wrong forms signature #760

Closed PavelPancocha closed 2 months ago

PavelPancocha commented 2 months ago

What version of Unfold are you using?

main

What version of Django are you using?

Any

What browser are you using?

Not relevant

Did you checked changelog/commit history, if the bug is not already fixed?

Yes

Did you searched other issues, if the bug is not already fixed?

Yes

Did you checked documentation?

Yes

Are you able to replicate the bug in the demo site?

Yes

Repository with reproduced bug

Not relevant

Describe your issue

The forms' signatures are not valid. Only one form is supposed to take request:

https://github.com/unfoldadmin/django-unfold/blob/main/src/unfold/forms.py#L56-L68

Other forms:

https://github.com/unfoldadmin/django-unfold/blob/main/src/unfold/forms.py#L69-L124

Do not take request as first arg. They take "data".

Why is it wrong:

How to fix

Change typing/signature of the forms, see: #752

lukasvinclav commented 2 months ago

Actually this is everything what you need, right?

764

PavelPancocha commented 2 months ago

@lukasvinclav yes, but I think having some coverage is always nice

lukasvinclav commented 2 months ago

Please test my PR, if it works for you. If everything is okay, I will merge it to main.

PavelPancocha commented 2 months ago

All fine, next time re-using my PR would feel more appropriate. But fine.

lukasvinclav commented 2 months ago

Why it should be more appropriate? What is the difference for you whos PR was accepted if it improves the package?

For comparison here are pull requests:

PavelPancocha commented 2 months ago

I do not want to start any blame war. I think, my PR had more value. If you feel, the tests/other stuff is unnecessary/should be done in separate PR, I am fine with that. That was not addressed, I didn't get, where the need for different PR come from. Thats all.

Thanks :)

lukasvinclav commented 2 months ago

The reasons for new PR come from:

At the end of the day, I decided to pick cherries from your PR, move them into one simple PR and ship it today so you can have updated main branch asap :-)

PavelPancocha commented 2 months ago

Ok, fine then. Just about typing - as you now wrote it, I got that User is much more specific, will not work for most of my projects as I use "Custom user" model: https://github.com/unfoldadmin/django-unfold/pull/764/files#r1771693202

PavelPancocha commented 2 months ago

About "personal preferences" - the best would be to have some sort of examples: How to write tests. I will do my best to match the style. I don't see any "specific" style in Unfold so far.