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

Rename min/max filters for clarity #958

Merged
merged 1 commit into from
Dec 6, 2017
Merged

Rename min/max filters for clarity #958

merged 1 commit into from
Dec 6, 2017

Conversation

Thibaut
Copy link
Contributor

@Thibaut Thibaut commented Dec 6, 2017

min and max were added in #954. The original implementation behaved as "min = limit a number to a minimum value" and "max = limit a number to a maximum value".

This makes sense in the context of Liquid, since a filter is generally interpreted as a verb/action, but is confusing in the context of programming languages in general, which tend to have min / max functions that return the minimum/maximum value passed as argument.

After realizing this, I flipped the behavior in 282d42f, before realizing that it's not that simple 😛

Given the confusion (I've asked multiple people and got different answers), I propose we rename the filters for clarity.

@nithinbekal @pushrax @davidcornu @fw42 @dylanahsmith thoughts?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@davidcornu
Copy link
Contributor

Of all the discussed options I think at_most and at_least make the most sense.

@fw42
Copy link
Contributor

fw42 commented Dec 6, 2017

Are we not concerned that this breaks backwards compatibility?

@Thibaut
Copy link
Contributor Author

Thibaut commented Dec 6, 2017

@fw42 this was added a few days ago. It hasn't been released yet.

@arafatkatze
Copy link

Yes, Thanks.
This seems better.

@nithinbekal
Copy link
Contributor

👍 This seems much better.

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.

None yet

6 participants