AndrewIngram / django-extra-views

Django's class-based generic views are awesome, let's have more of them.
MIT License
1.39k stars 172 forks source link

Add form_kwargs and get_form_kwargs for inline forms for solving issue #256 #257

Closed dw-liedji closed 3 weeks ago

dw-liedji commented 1 year ago

Using this Stack Overflow answer and this section of the Django documentation, I have solve the issue of missing form_kwargs as attribute of the formset_kwargs dictionary as mentioned in this section of the django-extra-views docs .

To make the process of passing parameter to inline forms more easier and consistent, I have additionally added the form_kwargs and the get_form_kwargs for passing parameters into each inline forms at runtime or not.

With these changes, to change the arguments which are passed into each form within the formset, two options are now available:

Here are some examples of using these news features. Note that, I use a fake Attribute class for the demonstration in theses examples, where the current user is passed into each inline forms:

class AttributeInline(InlineFormSetFactory):
    model = models.Attribute
    form_class = AttributeForm
    form_kwargs = {"user": 1}

Or override the get_form_kwargs for passing parameters of inline forms at runtime:

class AttributeInline(InlineFormSetFactory):
    model = models.Attribute
    form_class = AttributeForm

    def get_form_kwargs(self):
        form_kwargs = super(AttributeInline, self).get_form_kwargs()
        form_kwargs["user"] = self.request.user
        return form_kwargs

It is important to note that it can still be done using the get_formset_kwargs interface see docs. However, the form_kwargs declaration and get_form_kwargs interface are just more easier and consistent.

Thanks,

D. W. Liedji

sdolemelipone commented 1 year ago

Thanks for the contribution! Strange that form_kwargs isn't working as described in the docs as I'm sure that's something I use in my projects. I'll do some testing and check back.

What version of Django are you using?

sdolemelipone commented 1 year ago

Ah, sorry I misunderstood in my previous response that the get_formset_kwargs wasn't working for you. I see now that you're adding a more user friendly way to do this. I'll take a look over the changes and get back with any comments.

dw-liedji commented 1 year ago

Thank you @sdolemelipone for your quick response. The change made are working just fine in my project. How can I check the testings locally to improve the formatting for black and other failed tests ?

dw-liedji commented 1 year ago

This is the django and python versions that I am using:

Django Version: 4.1.4
Python Version: 3.8.8
sdolemelipone commented 1 year ago

Thank you @sdolemelipone for your quick response. The change made are working just fine in my project. How can I check the testings locally to improve the formatting for black and other failed tests ?

We should really add a pre-commit hook to do this automatically, I'll add that in a separate PR. In the meantime, using your local virtual environment you should run the following:

pip install black flake8 isort

Then run each of the below from the root directory of the project

black .
isort .
flake8 .

That will either fix the errors or tell you what needs fixing.

sdolemelipone commented 1 year ago

Hi @dw-liedji, I agree that having the form_kwargs tucked away inside formset_kwargs isn't very user friendly. Adding the class attribute seems like a good idea.

It's important that the old method of using formset_kwargs["form_kwargs"] still works. Could you adjust your code so that formset_kwargs["form_kwargs"] is only updated if get_form_kwargs() returns a non-empty dictionary?

After that could you add tests and documentation for the new form_kwargs attribute?

You can run tests by running pip install tox in your local environment then running the command tox from the project root. It will fail for versions of python which you don't have installed (also this is another way to run black/isort/flake8, e.g. tox -e black).

Don't worry about the failing python 3.5 / 3.6 tests, I think we need to remove those outdated versions from the test suite. I'll raise a separate PR for that.

Let me know if you need any more help/advice with all that.

dw-liedji commented 1 year ago

Hi @sdolemelipone and happy new year!

Thank you for your guide in testings. I would do it and make another push request.

Concerning yours questions:

  1. Could you adjust your code so that formset_kwargs["form_kwargs"] is only updated if get_form_kwargs() returns a non-empty dictionary? My answer: Yes, I could.
  2. After that could you add tests and documentation for the new form_kwargs attribute? My answer: Yes, I could. However, I would need your help to guide me on the reviewing the documentation that I would add.
dw-liedji commented 1 year ago

Hi @sdolemelipone ,

I have added a new commit. Please consider checking it out.

sdolemelipone commented 1 year ago

Hi @dw-liedji,

Please stick to using the same branch and pull request rather than creating new ones for further changes. That allows us to keep the changes in one place. Can you push your latest changes to the branch dw-liedji:bugfix/form_kwargs? No problem if you push many incremental commits, we can squash them before merging to the master branch.

I am happy to review any tests and documentation that you submit. If you aren't sure where to start, I can point you in the right direction after you have pushed your latest changes to this branch. Alternatively I'm happy to write tests/docs if you don't have time.

Thanks

sdolemelipone commented 3 weeks ago

Closed, replaced with #271.