Template for adding multiple op aliases to the same filters
#20
Conversation
…SQON reducer - adds `isSameFilter()` function to compare if `gt` and `>` are the same - updates tests to use mix of Symbol (`>`) and Letter (`gt`) op keys
justincorrigible
left a comment
justincorrigible
left a comment
There was a problem hiding this comment.
👍 the code overall looks great, of course. nice addition to facilitate contributions!
however, I found a couple things a bit confusing, and left some comments looking to discuss whether we can reach an agreement or workaround that helps prevent/reduce that.
... this PR is going to be super useful for what Evans is working on, and both the types and zod validators will help heaps once we integrate this in the Arranger monorepo 🎉
| isFilter, | ||
| } from './types/sqon'; | ||
| SQON, | ||
| type ArrayFilter, |
| @@ -1,107 +1,28 @@ | |||
| import { z as zod } from 'zod'; | |||
| import { matchesSchema } from '../utils/matchesSchema'; | |||
| import { | |||
There was a problem hiding this comment.
looks like these types are missing a verbatim import?
| import { | |
| import type { |
There was a problem hiding this comment.
following up on that, after going further into the file, and I'm honestly not sure how to say this without being mistaken as confrontational... so I'm just going to write my thought process, and hope you know I mean well:
- I look at line 4 in this file.
- in general terms, I expect
arrayFilterwould be the name for some "function that filters arrays" ("filter" being not very descriptive, but hey... a function that does something, and I know naming things is hard). - as this is a
types/*file, naming somethingArrayFilterreads to me as the type/signature for a function like the one I described above. - then I look at line 56, where it seems to be used as a template constant for validation in runtime —unless I'm misreading what that function does, which is possible while reviewing in GH; but constants have traditionally been named in upper snake case, e.g.
ARRAY_FILTER.so maybe it's not a constant either? - not a function's signature, not a constant, what does this initial capital mean? is that a zod executable schema? why doesn't it just say so in the name instead of having nonstandard casing?
- confoosed, I write this comment. sorry? please don't be upset.
- fin
Tl;DR: reading further down into this file, now I'm not entirely sure of what imports are types, classes/zod objects, or constants... I find the non-standard capitalization make the code truly hard to read without intellisense.
if it helps illustrate my current understanding of casing standards in JS, this is a somewhat succinct summary: https://medium.com/@sodiq.akanmu001/javascript-casing-conventions-a-must-know-for-every-developer-770a4474a7f0
There was a problem hiding this comment.
okay, went into the next file (types/sqonFilters), and there it looks like thisarrayFilter is indeed a zod schema of some sort, and the best I can think of is (and pretty please read this as "what would Justin do, he does not at all mean I have to like it"... not as "why is Justin attacking me, what a jerk, who is he to tell me how to do things" etc. please please please):
- this
arrayFilterexample, being a zod executable object is technically a function. - that means to me it gets the usual camelCase treatment, and adding
*Schemamakes it clearer what the variable contains. e.g.arrayFilterSchema. - perhaps
arrayFilterZodSchemaorarrayFilterZSchemawould be better? unclear. - it's not a type or a class, so I wouldn't use initial capital. that leads to confusion, in scopes/files where Z-usage is not obvious (such as reviewing PRs or onboarding new maintainers).
- it's technically not just a constant either, so upper snake doesn't apply either.
what do you think? am I making any sense?
I'm more than willing to be convinced otherwise if you have good arguments, but I'm just speaking from what I'm used to being convention in JS-land. again, sorry if this upsets you, but what's the point of PR reviews otherwise if not to give honest constructive feedback/thoughts?
| isScalarFilterKey, | ||
| isScalarFilterValue, | ||
| } from '../types/sqon'; | ||
| } from '../types'; |
There was a problem hiding this comment.
should all these be import type { as well? not sure anymore...
after seeing zod runtime stuff + types being used (which I really like, in case it's not clear), it intuitively feels like we'd do well establishing a clearer separation in folder structure, something like types/ vs schemas/ (or zodSchemas/?).
until we have intellisense in the PR reviewing flow (and even after that, if it ever happens), I believe that declarative distinction would reduce cognitive burden by orders of magnitude.
could become a nice pattern for future projects, if it works here
| // return false; | ||
| } | ||
|
|
||
| export function isSameFilter(filterA: FilterOperator, filterB: FilterOperator): boolean { |
There was a problem hiding this comment.
just realized, is filter a synonym to SQON? if not, what's the difference?
| isFilter, | ||
| } from '../types/sqon'; | ||
| LesserThanFilter, | ||
| type CombinationOperator, |
There was a problem hiding this comment.
in contrast, this works well, and naming convention matches my expectations.
truly hope you can understand my confusion...
Arranger supports SQONs that use
>orgtas theopvalue for GreaterThan filters, and other aliases. This creates a template for adding in aliases for a given filter type.Included here are the aliases:
>forgt<forltDetails
/src/types/sqonFilters.tsisSameFilterfunction to test if two filters are the same based on their op valueThere is quite a bit to manage when adding an alias to get this right so some documentation on maintaining these types is likely required.
Tickets