wagtail / wagtail-generic-chooser

A toolkit for custom chooser popups in Wagtail
BSD 3-Clause "New" or "Revised" License
116 stars 25 forks source link

API confusion #43

Open drcongo opened 2 years ago

drcongo commented 2 years ago

Hello. In the readme we have this text...

First, we customise the chooser view to expose a country URL parameter; to do this, we define a custom chooser_mixin_class for the viewset to use, and override its get_unfiltered_object_list method to filter by the country parameter.

So, if I understand this correctly, we override the get_unfiltered_object_list method to return a filtered object list. This feels like it's going to become very confusing for developers trying to understand a new code base, and maybe I'm missing something, but if we want that flexibility in this method, would it not be better to call it get_object_list?

gasman commented 2 years ago

As they say, the two hard problems in computer science are cache invalidation and naming things...

There is already a get_object_list method, which returns the list of objects to present back to the user, after applying any searching/filtering steps that happen inside the chooser (but before applying pagination). And indeed, this method works by taking get_unfiltered_object_list as its starting point before filtering it down to the final list.

In the countries example, we're notionally modifying the 'master' list of objects that the searching / filtering components within the chooser UI will apply to, so from the chooser's perspective, it is the 'unfiltered' list. Maybe a name like get_full_data_set would fit better... or maybe the confusing naming is a sign that this code should be refactored so that this 'initial pre-filtering' step isn't treated any differently from the filtering that happens within the chooser UI.