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

feat: add flash plugin #3145

Merged
merged 34 commits into from Mar 14, 2024
Merged

feat: add flash plugin #3145

merged 34 commits into from Mar 14, 2024

Conversation

euri10
Copy link
Contributor

@euri10 euri10 commented Feb 27, 2024

Description

  • added a flash plugin ala flask, uses the request state

Closes

Closes #1455, #2560

litestar/contrib/minijinja.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.25%. Comparing base (bfeb5e9) to head (ba71a7e).
Report is 103 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3145      +/-   ##
===========================================
+ Coverage    98.23%   98.25%   +0.02%     
===========================================
  Files          312      321       +9     
  Lines        14121    14518     +397     
  Branches      2430     2331      -99     
===========================================
+ Hits         13872    14265     +393     
- Misses         107      111       +4     
  Partials       142      142              

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

@euri10
Copy link
Contributor Author

euri10 commented Feb 28, 2024

diff --git a/docs/examples/templating/template_functions_minijinja.py b/docs/examples/templating/template_functions_minijinja.py
index 0fccdccb3..5655f92e5 100644
--- a/docs/examples/templating/template_functions_minijinja.py
+++ b/docs/examples/templating/template_functions_minijinja.py
@@ -26,4 +26,4 @@ def index() -> Template:
     return Template(template_name="index.html.minijinja")
 
 
-app = Litestar(route_handlers=[index], template_config=template_config)
+app = Litestar(route_handlers=[index], template_config=template_config, debug=True)
diff --git a/litestar/contrib/minijinja.py b/litestar/contrib/minijinja.py
index 6829ff45c..f7775a0c0 100644
--- a/litestar/contrib/minijinja.py
+++ b/litestar/contrib/minijinja.py
@@ -7,7 +7,6 @@ from typing import TYPE_CHECKING, Any, Mapping, Protocol, TypeVar, cast
 from typing_extensions import ParamSpec
 
 from litestar.exceptions import ImproperlyConfiguredException, MissingDependencyException, TemplateNotFoundException
-from litestar.plugins.flash import get_flashes as _get_flashes
 from litestar.template.base import (
     TemplateCallableType,
     TemplateEngineProtocol,
@@ -162,17 +161,24 @@ class MiniJinjaTemplateEngine(TemplateEngineProtocol["MiniJinjaTemplate", StateP
         return MiniJinjaTemplate(self.engine, template_name)
 
     def register_template_callable(
-        self, key: str, template_callable: TemplateCallableType[StateProtocol, P, T]
+        self, key: str, template_callable: TemplateCallableType[StateProtocol, P, T], transform: bool = False
     ) -> None:
         """Register a callable on the template engine.
 
         Args:
             key: The callable key, i.e. the value to use inside the template to call the callable.
             template_callable: A callable to register.
+            transform: If True, the callable will be transformed to receive a ``StateProtocol`` instance as first argument.
 
         Returns:
             None
         """
+
+        def is_decorated(func):
+            return hasattr(func, "__wrapped__") or func.__name__ not in globals()
+
+        if not is_decorated(template_callable) or transform:  # or comes from a plugin...
+            template_callable = _transform_state(template_callable)
         self.engine.add_global(key, pass_state(template_callable))
 
     def render_string(self, template_string: str, context: Mapping[str, Any]) -> str:
@@ -206,9 +212,6 @@ def _minijinja_from_state(func: Callable, state: StateProtocol, *args: Any, **kw
     return cast(str, func(template_context, *args, **kwargs))
 
 
-get_flashes = _transform_state(_get_flashes)
-
-
 def __getattr__(name: str) -> Any:
     if name == "minijinja_from_state":
         warn_deprecation(
diff --git a/litestar/plugins/flash.py b/litestar/plugins/flash.py
index 0ffde4a55..4b34d7897 100644
--- a/litestar/plugins/flash.py
+++ b/litestar/plugins/flash.py
@@ -1,10 +1,10 @@
-import contextlib
 from dataclasses import dataclass
 from enum import Enum
 from typing import Any, Mapping
 
 from litestar.config.app import AppConfig
 from litestar.connection import ASGIConnection
+from litestar.contrib.minijinja import MiniJinjaTemplateEngine
 from litestar.plugins import InitPluginProtocol
 from litestar.template import TemplateConfig
 from litestar.template.base import _get_request_from_context
@@ -35,18 +35,10 @@ class FlashPlugin(InitPluginProtocol):
 
     def on_app_init(self, app_config: AppConfig) -> AppConfig:
         """Register the message callable on the template engine instance."""
-        with contextlib.suppress(ImportError):
-            # minijinja needs a different callable here
-            from litestar.contrib.minijinja import MiniJinjaTemplateEngine
-            from litestar.contrib.minijinja import get_flashes as minijinja_get_flashes
-
-            if isinstance(self.config.template_config.engine_instance, MiniJinjaTemplateEngine):
-                self.config.template_config.engine_instance.register_template_callable(
-                    "get_flashes", minijinja_get_flashes
-                )
-                return app_config
-
-        self.config.template_config.engine_instance.register_template_callable("get_flashes", get_flashes)
+        if isinstance(self.config.template_config.engine_instance, MiniJinjaTemplateEngine):
+            self.config.template_config.engine_instance.register_template_callable("get_flashes", get_flashes, True)
+        else:
+            self.config.template_config.engine_instance.register_template_callable("get_flashes", get_flashes)
         return app_config

@provinzkraut this is a small diff between your semi-ugly fix and my semi-ugly one.
Honestly I dont have a real preference, my gut feeling just tells me somehow we should do more in the minijinja implementation to detect if the callable we wanna register is or is not wrapped correctly, or defer that responsability to the caller (but would that be breaking ?)
that's an interesting case because imho the plugin should not have to do that switch dance in the app_init and that should be handled oob in the implementation but I dont see an easy way in minijinja's case to tell hey I've been called by a plugin, pass me the state

@euri10 euri10 force-pushed the flash branch 2 times, most recently from 89c76c0 to 05532c5 Compare March 4, 2024 07:45
@euri10 euri10 changed the base branch from main to develop March 4, 2024 07:46
@euri10
Copy link
Contributor Author

euri10 commented Mar 4, 2024

it would be somewhat better to pass an enum of the possible message categories to the FlashConfig so that the category passed to flash(message, category) belongs to that enum only, but I dont know if this is feasible.
the mypy errors seems unrelated to me

@euri10 euri10 marked this pull request as ready for review March 4, 2024 14:57
@euri10 euri10 requested review from a team as code owners March 4, 2024 14:57
Copy link
Member

@JacobCoffee JacobCoffee left a comment

Choose a reason for hiding this comment

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

this is awesome.

I have put up euri10#1 for documentation - i can work on the placeholder data if you dont feel like if it gets merged

litestar/plugins/flash.py Show resolved Hide resolved
@euri10
Copy link
Contributor Author

euri10 commented Mar 4, 2024

this is awesome.

I have put up euri10#1 for documentation - i can work on the placeholder data if you dont feel like if it gets merged

sorry I'm slow and didn't understood at first it was on my fork :) need to wake up !

@euri10 euri10 marked this pull request as draft March 4, 2024 17:45
@euri10
Copy link
Contributor Author

euri10 commented Mar 4, 2024

working on docs, back to draft mode

@JacobCoffee
Copy link
Member

Sorry i caught this late, feel free to disregard - i just thought about the API docs and added those but ended up doing more.

not 100% on the validity.

euri10#2

anyways its working good for me in testing ty for your PR :)

@euri10 euri10 marked this pull request as ready for review March 8, 2024 10:26
@JacobCoffee
Copy link
Member

commit tree looks weird

@provinzkraut
Copy link
Member

commit tree looks weird

Needs a rebase

@euri10
Copy link
Contributor Author

euri10 commented Mar 10, 2024

honestly I dont get the rebase and stuff, now we have a PR with 127 changes :(
should I just close this and redo from scratch ?

@euri10
Copy link
Contributor Author

euri10 commented Mar 10, 2024

are we better now ?

@provinzkraut
Copy link
Member

are we better now ?

Looks good! You did it! :)

@euri10
Copy link
Contributor Author

euri10 commented Mar 10, 2024

are we better now ?

Looks good! You did it! :)

@Alc-Alc did ;)

@JacobCoffee JacobCoffee added this to the 2.8.0 milestone Mar 10, 2024
@JacobCoffee
Copy link
Member

@JacobCoffee JacobCoffee assigned JacobCoffee and euri10 and unassigned JacobCoffee Mar 10, 2024
@euri10
Copy link
Contributor Author

euri10 commented Mar 10, 2024

Failing types arent even related to PR :( litestar-org/litestar/actions/runs/8223737329/job/22486749530?pr=3145

I had a pdm.lock change, hopefully this will solve it

Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3145

@JacobCoffee JacobCoffee merged commit 817f518 into litestar-org:develop Mar 14, 2024
20 checks passed
provinzkraut added a commit that referenced this pull request Mar 16, 2024
* fix minijinja.py to mimic what we do on other templates so that any regitered callable can access the scope ie _transform_state is in the wrong spot

* added a flash function and its pending get_flashes one to be used in templates

* can't use StrEnum

* can't use StrEnum in tests as well

* remove StrEnum entirely

* enum not iterable ?

* silence mypy on this one, python/mypy#16936 related ?

* same

* TIL about ScopeState

* Silence another mypy error that works in test_temmplate...

* semi-ugly "fix"

* another way of looking at minjinja implementation

* Yes I debug with print sometimes

* still fails because get_flashes is not decorted properly in minijinja implementation, there should be a way to detect

* another "working" option, no preference vs other options

* another "working" option, no preference vs other options

* Removed the transformed kwrd, would be breaking probably

* Removed debug

* Removed docstring leftover

* Mypy

* Demonstrated superior-mypy-foo-skills by using type ignore

* Removed FLashCategory from plugin, shouldn't the app_init get it passed in the config ??

* docs: add plugin docs for flash messages

* docs: add plugin docs for flash messages

* Update docs/usage/index.rst

* fix: update file path

* fix: make sphinx gods happy

* docs: add more documentation

* docs: move api doc plugins into directory

* revert pdm.lock to it's original state

* living the dangerous life of not testing code

* typo

---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
provinzkraut added a commit that referenced this pull request Mar 16, 2024
* fix minijinja.py to mimic what we do on other templates so that any regitered callable can access the scope ie _transform_state is in the wrong spot

* added a flash function and its pending get_flashes one to be used in templates

* can't use StrEnum

* can't use StrEnum in tests as well

* remove StrEnum entirely

* enum not iterable ?

* silence mypy on this one, python/mypy#16936 related ?

* same

* TIL about ScopeState

* Silence another mypy error that works in test_temmplate...

* semi-ugly "fix"

* another way of looking at minjinja implementation

* Yes I debug with print sometimes

* still fails because get_flashes is not decorted properly in minijinja implementation, there should be a way to detect

* another "working" option, no preference vs other options

* another "working" option, no preference vs other options

* Removed the transformed kwrd, would be breaking probably

* Removed debug

* Removed docstring leftover

* Mypy

* Demonstrated superior-mypy-foo-skills by using type ignore

* Removed FLashCategory from plugin, shouldn't the app_init get it passed in the config ??

* docs: add plugin docs for flash messages

* docs: add plugin docs for flash messages

* Update docs/usage/index.rst

* fix: update file path

* fix: make sphinx gods happy

* docs: add more documentation

* docs: move api doc plugins into directory

* revert pdm.lock to it's original state

* living the dangerous life of not testing code

* typo

---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
provinzkraut added a commit that referenced this pull request Mar 16, 2024
* fix minijinja.py to mimic what we do on other templates so that any regitered callable can access the scope ie _transform_state is in the wrong spot

* added a flash function and its pending get_flashes one to be used in templates

* can't use StrEnum

* can't use StrEnum in tests as well

* remove StrEnum entirely

* enum not iterable ?

* silence mypy on this one, python/mypy#16936 related ?

* same

* TIL about ScopeState

* Silence another mypy error that works in test_temmplate...

* semi-ugly "fix"

* another way of looking at minjinja implementation

* Yes I debug with print sometimes

* still fails because get_flashes is not decorted properly in minijinja implementation, there should be a way to detect

* another "working" option, no preference vs other options

* another "working" option, no preference vs other options

* Removed the transformed kwrd, would be breaking probably

* Removed debug

* Removed docstring leftover

* Mypy

* Demonstrated superior-mypy-foo-skills by using type ignore

* Removed FLashCategory from plugin, shouldn't the app_init get it passed in the config ??

* docs: add plugin docs for flash messages

* docs: add plugin docs for flash messages

* Update docs/usage/index.rst

* fix: update file path

* fix: make sphinx gods happy

* docs: add more documentation

* docs: move api doc plugins into directory

* revert pdm.lock to it's original state

* living the dangerous life of not testing code

* typo

---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
@euri10 euri10 deleted the flash branch March 16, 2024 10:00
provinzkraut added a commit that referenced this pull request Mar 17, 2024
* fix minijinja.py to mimic what we do on other templates so that any regitered callable can access the scope ie _transform_state is in the wrong spot

* added a flash function and its pending get_flashes one to be used in templates

* can't use StrEnum

* can't use StrEnum in tests as well

* remove StrEnum entirely

* enum not iterable ?

* silence mypy on this one, python/mypy#16936 related ?

* same

* TIL about ScopeState

* Silence another mypy error that works in test_temmplate...

* semi-ugly "fix"

* another way of looking at minjinja implementation

* Yes I debug with print sometimes

* still fails because get_flashes is not decorted properly in minijinja implementation, there should be a way to detect

* another "working" option, no preference vs other options

* another "working" option, no preference vs other options

* Removed the transformed kwrd, would be breaking probably

* Removed debug

* Removed docstring leftover

* Mypy

* Demonstrated superior-mypy-foo-skills by using type ignore

* Removed FLashCategory from plugin, shouldn't the app_init get it passed in the config ??

* docs: add plugin docs for flash messages

* docs: add plugin docs for flash messages

* Update docs/usage/index.rst

* fix: update file path

* fix: make sphinx gods happy

* docs: add more documentation

* docs: move api doc plugins into directory

* revert pdm.lock to it's original state

* living the dangerous life of not testing code

* typo

---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
provinzkraut added a commit that referenced this pull request Mar 18, 2024
* fix minijinja.py to mimic what we do on other templates so that any regitered callable can access the scope ie _transform_state is in the wrong spot

* added a flash function and its pending get_flashes one to be used in templates

* can't use StrEnum

* can't use StrEnum in tests as well

* remove StrEnum entirely

* enum not iterable ?

* silence mypy on this one, python/mypy#16936 related ?

* same

* TIL about ScopeState

* Silence another mypy error that works in test_temmplate...

* semi-ugly "fix"

* another way of looking at minjinja implementation

* Yes I debug with print sometimes

* still fails because get_flashes is not decorted properly in minijinja implementation, there should be a way to detect

* another "working" option, no preference vs other options

* another "working" option, no preference vs other options

* Removed the transformed kwrd, would be breaking probably

* Removed debug

* Removed docstring leftover

* Mypy

* Demonstrated superior-mypy-foo-skills by using type ignore

* Removed FLashCategory from plugin, shouldn't the app_init get it passed in the config ??

* docs: add plugin docs for flash messages

* docs: add plugin docs for flash messages

* Update docs/usage/index.rst

* fix: update file path

* fix: make sphinx gods happy

* docs: add more documentation

* docs: move api doc plugins into directory

* revert pdm.lock to it's original state

* living the dangerous life of not testing code

* typo

---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
provinzkraut added a commit that referenced this pull request Mar 18, 2024
---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
JacobCoffee added a commit that referenced this pull request Mar 22, 2024
---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
provinzkraut added a commit that referenced this pull request Mar 29, 2024
---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
provinzkraut added a commit that referenced this pull request Mar 29, 2024
---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
provinzkraut added a commit that referenced this pull request Mar 30, 2024
---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
JacobCoffee added a commit that referenced this pull request Apr 5, 2024
---------

Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Co-authored-by: Jacob Coffee <jacob@z7x.org>
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.

Add support for "Message Flashing"
3 participants