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

Allow None on view_on_site #2419

Merged
merged 1 commit into from
Oct 25, 2024
Merged

Allow None on view_on_site #2419

merged 1 commit into from
Oct 25, 2024

Conversation

nijel
Copy link
Contributor

@nijel nijel commented Oct 24, 2024

I have made things!

Related issues

This makes it consistent with get_view_on_site_url signature where it is used.

This makes it consistent with `get_view_on_site_url` signature where it is used.
@@ -139,7 +139,7 @@ class BaseModelAdmin(Generic[_ModelT]):
def has_view_or_change_permission(self, request: HttpRequest, obj: _ModelT | None = ...) -> bool: ...
def has_module_permission(self, request: HttpRequest) -> bool: ...
@property
def view_on_site(self) -> Callable[[_ModelT], str] | bool: ...
def view_on_site(self) -> Callable[[_ModelT], str | None] | bool: ...
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but the linked code doesn't deal with the return value from the callable at all.

view_on_site can be either a boolean, or a callable returning a string or None. When callable returns None, get_view_on_site_url will return None, what is a valid return value here (as can be seen in the code you linked, which returns None as well when view_on_site is false).

Copy link
Member

Choose a reason for hiding this comment

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

My bad, I misread the type :)
Thanks for the clarification.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks!

@sobolevn sobolevn merged commit 8de9d9a into typeddjango:master Oct 25, 2024
40 checks passed
@nijel nijel deleted the patch-1 branch October 25, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants