MushroomObserver / mushroom-observer

A website for sharing observations of mushrooms.
https://mushroomobserver.org
MIT License
77 stars 25 forks source link

Query params and scopes, but not API params: Switch all `has_` params to `with_` #2097

Closed nimmolo closed 3 months ago

nimmolo commented 3 months ago

Changes Query boolean param names from has_description to with_description, etc., and changes a few model scopes to match. (Mostly, the model scopes are already named like with_description.)

It's OCD but it gets the params in alignment with the model scopes, which may be useful when refactoring Query to use model scopes. This gets us closer to compliance with a Rubocop rule against has_xxx method names. (There's no Rubocop rule against has_ as a param name. Of course the scopes could use the has_ pattern, contra Rubocop, but no need to pile up violations.)

In the spirit of standardizing and reducing surprises with these param names, this also adds has_ to a few PatternSearch params that are also boolean, like has_images, has_sequences and has_specimen. (formerly images, sequences, and specimen). The rest of PatternSearch is left as currently.

Implications of this PR:

coveralls commented 3 months ago

Coverage Status

coverage: 94.396% (+0.001%) from 94.395% when pulling 0fea0e731f45e79de15d66aed1996fc034f94246 on query-has-with into e698ddfe7e0c6e58abeddf20a9a6223f7de0c61e on main.

nimmolo commented 3 months ago

@JoeCohen I added this message at the top of the flash, if there's a has_ term.

We've changed some terms. Please try "with#{thing}" instead of "has#{thing}".

nimmolo commented 3 months ago

@JoeCohen Thanks for trying it out! It should be pretty easy in the future to make a popup form that will populate these search terms and form the entire search string, so people don't have to type them all out.

nimmolo commented 3 months ago

I just realized (belatedly) that the pattern search params are no more coupled to the Query params than the API params are. They both go through a step of mapping param: other_param.

To heck with changing these, let's keep them the same as they currently are. Now if i can just untangle them...

nimmolo commented 3 months ago

Wrote a script to update the user content_filter preference strings in the db, replacing has_images and has_specimen with with_images and with_specimen.

The ContentFilter stuff maps straight onto Query methods, so these two have to change.

nimmolo commented 3 months ago

@JoeCohen - the param changes are all now under the hood, not affecting users, except for the three changes to PatternSearch params: images, specimen and sequence.

I understand why these params were abbreviated in the first place: no one is going to search by a specific image id, so images:foo is theoretically unambiguous, unlike comments:foo, where the param is describing a searched phrase in the comments, so the boolean has to be has_comments:true. The same for specimens and sequences (no one is going to search by sequence:DAGDAD.) Maybe for minimum feather-ruffling, we keep these as is.

But to me, not a PatternSearch power user, i do prefer this change to has_images, has_specimen and has_sequence because all the many other Boolean params have has_. It seems easier to remember.