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

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

Closed
wants to merge 1 commit into from

Conversation

dw-liedji
Copy link

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:

  • The first approach is done either by the form_kwargs argument passed in to the FormSet constructor as previously mentionned in the documentation (I have solved the bug issue);
  • The second approach (newly added) is done by simply declaring the form_kwargs option in the inline formset class or override the get_form_kwargs for passing parameters of inline forms at runtime.

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

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

Django Version: 4.1.4
Python Version: 3.8.8

@sdolemelipone
Copy link
Collaborator

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
Copy link
Collaborator

sdolemelipone commented Jan 1, 2023

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
Copy link
Author

dw-liedji commented Jan 3, 2023

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
Copy link
Author

dw-liedji commented Jan 3, 2023

Hi @sdolemelipone ,

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

@sdolemelipone
Copy link
Collaborator

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
Copy link
Collaborator

Closed, replaced with #271.

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