Fixed #13376: Added kwargs argument to messages API to allow extension of functionality to the Storage & Message class.#18332
Conversation
| the lowercase version of the name of the associated constant, but this | ||
| can be changed if you need by using the :setting:`MESSAGE_TAGS` setting. | ||
|
|
||
| * ``extra_kwargs``: This variable accepts all further keyword arguments |
There was a problem hiding this comment.
I'm not entirely happy with this, but believe it should be documented. Perhaps as a new section titled something like: Custom Storage & Message classes
There was a problem hiding this comment.
Also needs a ..versionchanged note and reference in the release notes
Good point, we have a bit on BaseStorage and how you theoretically can make a custom storage (https://docs.djangoproject.com/en/5.0/ref/contrib/messages/#django.contrib.messages.storage.base.BaseStorage) adding something in a section there demonstrating the value of this would really help
If you make a really simple example, that might also be useful for writing a test
| self.level = int(level) | ||
| self.message = message | ||
| self.extra_tags = extra_tags | ||
| self.extra_kwargs = kwargs |
There was a problem hiding this comment.
I'm not entirely happy with this line. The variable name is line with extra_tags, but we don't necessarily need to do anything here, so it could be removed. The point is that the Message class just accepted **kwargs
There was a problem hiding this comment.
ResolverMatch has extra_kwargs which might inspire you
You might prefer extra_kwargs=None in the signature than **kwargs (not sure)
I see you have added some docs (which is great), this needs a test
|
Hi @nanorepublica! Thank you for keeping up with contributing! 🌟 I see that you have, intentionally perhaps, not checked the "has patch" flag in the ticket. I just wanted to be explicit saying that in order for this PR to be listed as "needing review" in the Django Development Dashboard, the flag needs to be set when this is ready for review. No rush though! |
|
@nessita Thanks for this! I will be checking that flag now! |
sarahboyce
left a comment
There was a problem hiding this comment.
Thank you for this @nanorepublica - I have initial comments 👍
django/contrib/messages/api.py
Outdated
|
|
||
|
|
||
| def add_message(request, level, message, extra_tags="", fail_silently=False): | ||
| def add_message(request, level, message, extra_tags="", fail_silently=False, **kwargs): |
There was a problem hiding this comment.
add_message is documented and we will need to update the signature here.
You will need to add a .. versionchanged:: 5.2 note about the signature change and a release note in 5.2
You can also want to update the add_message docs to be more specific about the signatures of debug, info etc
| self.level = int(level) | ||
| self.message = message | ||
| self.extra_tags = extra_tags | ||
| self.extra_kwargs = kwargs |
There was a problem hiding this comment.
ResolverMatch has extra_kwargs which might inspire you
You might prefer extra_kwargs=None in the signature than **kwargs (not sure)
I see you have added some docs (which is great), this needs a test
| the lowercase version of the name of the associated constant, but this | ||
| can be changed if you need by using the :setting:`MESSAGE_TAGS` setting. | ||
|
|
||
| * ``extra_kwargs``: This variable accepts all further keyword arguments |
There was a problem hiding this comment.
Also needs a ..versionchanged note and reference in the release notes
Good point, we have a bit on BaseStorage and how you theoretically can make a custom storage (https://docs.djangoproject.com/en/5.0/ref/contrib/messages/#django.contrib.messages.storage.base.BaseStorage) adding something in a section there demonstrating the value of this would really help
If you make a really simple example, that might also be useful for writing a test
914d468 to
85415ff
Compare
| def get_message_class(self): | ||
| return self.message_class | ||
|
|
||
| def get_message(self, *args, **kwargs): |
There was a problem hiding this comment.
The function name feels slightly misleading to me on a class which is a storage wrapper - it implies to me that an existing message is being fetched from the storage rather than a new message created (for later storage).
There was a problem hiding this comment.
Yep, your right. This represents a possibly refactoring to allow custom Message classes, but in order to do that we would need to use these classes throughout the application which isn't the case right now. I'll get this removed for this PR.
Trac ticket number
ticket-13376
Branch description
This branch is a middle road for the mentioned Trac ticket. While not implementing the full requirements of the ticket it modifies the existing API in a backwards compatiable manner to either allow the ticket to be implemented in core or as a third-party package.
This also opens up the messages API to extensibility without further changes to core.
Checklist
mainbranch.