Simplified/centralised version dropdowns population (#1500) #1510
Conversation
daveoconnor
force-pushed
the
doc/1500-context-versions-consolidation
branch
from
60d3fa8
to
98313d6
Compare
libraries/mixins.py
Outdated
| @@ -41,8 +42,8 @@ def get_context_data(self, **kwargs): | |||
| if context["version_str"] == LATEST_RELEASE_URL_PATH_STR: | |||
| context["selected_version"] = context["current_version"] | |||
| elif context["version_str"]: | |||
| context["selected_version"] = Version.objects.get( | |||
| slug=context["version_str"] | |||
| context["selected_version"] = get_object_or_404( | |||
There was a problem hiding this comment.
Oh I don't think I've seen this used outside of a get/post/dispatch method, but it makes sense that it would bubble up. Nice.
| response = tp.get(url) | ||
| tp.response_404(response) | ||
| assert ( | ||
| "There was no version of the Boost.MultiArray library in the 1.70.0 version of " |
There was a problem hiding this comment.
Might be nice to make this dynamic.
| "There was no version of the Boost.MultiArray library in the 1.70.0 version of " | |
| f"There was no version of the Boost.MultiArray library in the {old_version.display_name} version of " |
| @@ -89,6 +89,7 @@ | |||
| urlpatterns = ( | |||
| [ | |||
| path("", HomepageView.as_view(), name="home"), | |||
| # todo: can we get rid of this? | |||
| from typing import ForwardRef | ||
|
|
||
| Library = ForwardRef("Library") | ||
| Version = ForwardRef("Version") |
There was a problem hiding this comment.
This is fine, but if you from __future__ import annotations, you can skip this and just use the class names directly in type annotations, thanks to PEP 649, which will land in 3.14.
There was a problem hiding this comment.
Also, if you prefer not to do the future thing, you can just use the strings directly in type hints as of... I think 3.10?
There was a problem hiding this comment.
OK, the second commit is interesting. I'm sort of on the fence about it, as you are.
I think #1500 has the ultimate idea of having a site-wide version dropdown, and I'm not sure the second commit gets us closer to that.
| version_path_kwargs = { | ||
| "release-detail": {}, | ||
| "libraries-list": { | ||
| "filter_out_has_no_libraries": True, | ||
| "force_version_inclusion": self.extra_context["current_version"], | ||
| }, | ||
| "library-detail": { | ||
| "flag_versions_without_library": partial( | ||
| get_object_or_404, Library, slug=self.kwargs.get("library_slug") | ||
| ) | ||
| }, | ||
| }.get(self.request.resolver_match.view_name, {}) | ||
| # we need this step to process any partials | ||
| processed_version_path_kwargs = { | ||
| key: (value() if callable(value) else value) | ||
| for key, value in version_path_kwargs.items() | ||
| } |
There was a problem hiding this comment.
This is cool, but... I think an if/else statement would be much more direct?
| version_path_kwargs = { | |
| "release-detail": {}, | |
| "libraries-list": { | |
| "filter_out_has_no_libraries": True, | |
| "force_version_inclusion": self.extra_context["current_version"], | |
| }, | |
| "library-detail": { | |
| "flag_versions_without_library": partial( | |
| get_object_or_404, Library, slug=self.kwargs.get("library_slug") | |
| ) | |
| }, | |
| }.get(self.request.resolver_match.view_name, {}) | |
| # we need this step to process any partials | |
| processed_version_path_kwargs = { | |
| key: (value() if callable(value) else value) | |
| for key, value in version_path_kwargs.items() | |
| } | |
| version_path_kwargs = {} | |
| if self.request.resolver_match.view_name == "libraries-list": | |
| version_path_kwargs = { | |
| "filter_out_has_no_libraries": True, | |
| "force_version_inclusion": self.extra_context["current_version"], | |
| } | |
| elif self.request.resolver_match.view_name == "library-detail": | |
| library = get_object_or_404(Library, slug=self.kwargs.get("library_slug")) | |
| version_path_kwargs = { | |
| "flag_versions_without_library": library | |
| } |
This is related to ticket #1500
I did this as two separate commits and it's probably a good idea to evaluate this PR that way:
get_dropdown_versions(néeversion_dropdown/version_dropdown_strict) with args.I chose to do point 2 in the BoostVersionMixin so we can use the data in a
dispatch()or other similar method if needed instead of being limited to it only being set for the templates, and that potentially causing different datasets being populated or used by unfamiliar devs in the future.I'm not sure I like this, which is why I set it as two separate commits so it can be evaluated separately more easily. I think it might just be making the code more complicated for not a whole lot of benefit and it's not really saving us much in terms of queries, we need the partial to do one we wouldn't need otherwise because
get_object()would otherwise provide that Library object, as you can see by the difference, but we can discuss it on Monday.I also think we can probably get rid of
path("homepage-beta/", HomepageBetaView.as_view(), name="home-beta"),and the related code, that uses a versions dataset but seems like it's not referenced anywhere and hasn't been discussed on the discord channel in over a year.