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

clarify interpretation of less and greater as before/after #461

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

combinatorist
Copy link

@combinatorist combinatorist commented Dec 22, 2024

I welcome edits / rephrasing. This is just the simplest change to make it clear to me, but it might only muddle it for others.

There might also be other places this clarification should be made - IDK.

Copy link

netlify bot commented Dec 22, 2024

Deploy Preview for grist-help-preview ready!

Name Link
🔨 Latest commit 08247b3
🔍 Latest deploy log https://app.netlify.com/sites/grist-help-preview/deploys/679c4349bfe3a1000861c791
😎 Deploy Preview https://deploy-preview-461--grist-help-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

- __`ge`__: (greater than or equal to) find nearest record with sort values >= the given values
- __`eq`__: (equal to) find nearest record with sort values == the given values
- __`lt`__: (less than) find nearest (last) record with sort values < (before) the given values
- __`le`__: (less than or equal to) find nearest record with sort values <= (last before or first equal) the given values
Copy link
Member

Choose a reason for hiding this comment

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

I think it should actually be "last that's before or equal" -- if you have multiple equal values to the one you are looking up, it should return the last of these.

Copy link
Author

Choose a reason for hiding this comment

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

Feel free to edit this PR to so the phrasing is more accurate. I was just pointing out that it wasn't clear to without these details. If it feels cleaner to you, maybe you could these alternative explanations somewhere else and link to that.

- __`lt`__: (less than) find nearest (last) record with sort values < (before) the given values
- __`le`__: (less than or equal to) find nearest record with sort values <= (last before or first equal) the given values
- __`gt`__: (greater than) find nearest (first) record with sort values > (after) the given values
- __`ge`__: (greater than or equal to) find nearest (first) record with sort values >= (equal or after) the given values
Copy link
Member

Choose a reason for hiding this comment

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

this one should be "first equal or after", though I think this is clear as is. Maybe le can be phrased the same way.

@dsagal dsagal force-pushed the feature/le-ge-as-before-after branch from 1c829f3 to e53a3c0 Compare January 30, 2025 04:21
@dsagal
Copy link
Member

dsagal commented Jan 30, 2025

Sorry to be slow. I think the extra explanation paragraph you added clarifies things nicely, so I simplified a bit the editing of the individual methods. If it looks good to you, I'll merge in. Preview here.

@dsagal dsagal requested a review from nataliemisasi January 30, 2025 18:34
Copy link
Collaborator

@nataliemisasi nataliemisasi left a comment

Choose a reason for hiding this comment

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

Made some updates to the formatting (capitalization, punctuation) to match formatting of list found here: https://deploy-preview-461--grist-help-preview.netlify.app/functions/#datedif

Added filler words because the existing phrasing requires the reader to mentally fill in gaps, making it harder to process. Since the concept is already a bit complex, including these filler words makes it easier to understand at a glance.

Comment on lines 130 to 136
- __`lt`__: ("less than") find nearest (last) record with sort values before the given values
- __`le`__: ("less than or equal to") find last record with sort values equal to or before
the given values
- __`gt`__: ("greater than") find nearest (first) record with sort values after the given values
- __`ge`__: ("greater than or equal to") find first record with sort values equal to or after
the given values
- __`eq`__: ("equal to") find the first record with sort values equal to the given values
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- __`lt`__: ("less than") find nearest (last) record with sort values before the given values
- __`le`__: ("less than or equal to") find last record with sort values equal to or before
the given values
- __`gt`__: ("greater than") find nearest (first) record with sort values after the given values
- __`ge`__: ("greater than or equal to") find first record with sort values equal to or after
the given values
- __`eq`__: ("equal to") find the first record with sort values equal to the given values
- __`lt`__: ("less than") Finds the nearest (last) record where the sort values are **before** the given values.
- __`le`__: ("less than or equal to") Finds the last record where the sort values are **equal to or before**
the given values.
- __`gt`__: ("greater than") Finds the nearest (first) record where the sort values are **after** the given values.
- __`ge`__: ("greater than or equal to") Finds the first record where the sort values are **equal to or after**
the given values.
- __`eq`__: ("equal to") Finds the first record where the sort values are **equal to** the given values.

Copy link
Member

Choose a reason for hiding this comment

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

Done, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants