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

limitTo filter should have a left and right bound. #5355

Closed
xieranmaya opened this issue Dec 10, 2013 · 27 comments
Closed

limitTo filter should have a left and right bound. #5355

xieranmaya opened this issue Dec 10, 2013 · 27 comments

Comments

@xieranmaya
Copy link
Contributor

@xieranmaya xieranmaya commented Dec 10, 2013

using like this:
{{'abcdefg' | limitTo:2:5}}
output:
cde
useful when paging sth..

@ghost ghost assigned IgorMinar Dec 21, 2013
@IgorMinar
Copy link
Member

@IgorMinar IgorMinar commented Dec 21, 2013

you can do this by using limitTo twice: {{ 'abscdefg' | limitTo:-2 | limitTo:3}}

it would be nice to have the api you are suggesting (except the second argument should be 3 in your example) but it might be confusing considering the existing api.

if you can work out the details and ensure that the api is not confusing we could consider it, especially if the code change required is tiny and someone from community submits a PR with tests and docs.

@xieranmaya
Copy link
Contributor Author

@xieranmaya xieranmaya commented Dec 21, 2013

it's easy to keep compatibility, you can do it like this
if limitTo filter have only one parameter, it denotes the length and behave like before
if limitTo filter have two parameters, it denotes the start and the length, and do the new cut function.
actually I have already implement a custom filter to do that

.filter('limit', function(){
    return function (input, left, right) {
        if(input === undefined || left === undefined) return input;
        if(right === undefined){
            right = left;
            left =  0;
        }
        return input.slice(left,right);//string and array all have this method
    };
});

it's simply to change the logic to limit:left:length

@gnomeontherun
Copy link
Contributor

@gnomeontherun gnomeontherun commented Jan 23, 2014

I'd like to suggest that to maintain current functionality the following be done. Leave the current first value as the limit, and add another parameter to set an offset. This would be analogous to SQL limit statements for those familiar with the syntax.

$scope.limit = 3;
$scope.offset = 2;

...

{{ [1,2,3,4,5,6,7,8,9] | filterTo:limit:offset }}

The only interesting caveat I can think of is that right now limit can be negative. That doesn't make sense to me when you add an offset value, so I'd suggest that if an offset is present then the limit should be always positive. I've already implemented this, and would make a PR if this makes sense.

@craigsssmith
Copy link

@craigsssmith craigsssmith commented Feb 19, 2014

How about if the API were to match PHP's substr(...) method, which is well established AND supports negative values for both the 'start' and 'length' parameters in an intuitive way?

@revolunet
Copy link
Contributor

@revolunet revolunet commented Feb 21, 2014

why not a new "slice" filter ? shoule be more explicit, less confusing

gnomeontherun pushed a commit to gnomeontherun/angular.js that referenced this issue Feb 21, 2014
This creates a new filter that passes a string or array to native
slice() method. This could be useful for pagination, because the
limitTo filter only allows a limit, not a start and ending position.
Creating a new filter is more intuitive and reflective of native
Javascript rather than extending limitTo. angular#5355
gnomeontherun pushed a commit to gnomeontherun/angular.js that referenced this issue Feb 21, 2014
This creates a new filter that passes a string or array to native slice() method. This is be useful for pagination or extracting a portion of a string or array. It differs from the limitTo filter because limitTo only has one bound, a starting position. Creating a new filter is more intuitive and reflective of native Javascript rather than extending limitTo.

It should close issue angular#5355.
@andytompkins
Copy link

@andytompkins andytompkins commented Mar 16, 2014

.filter('startWith', [function() {
    return function(input, index) {
        return input.slice(parseInt(index, 10));
    };
}]);
{{ 'abscdefg' | startWith:4 | limitTo:3}}
@btford btford removed the gh: issue label Aug 20, 2014
@mgcrea
Copy link
Contributor

@mgcrea mgcrea commented Oct 21, 2014

I'd agree with @revolunet about a new slice filter, limitTo with two args seems a bit awkward.

@remisb
Copy link

@remisb remisb commented Jan 21, 2015

+1 for new slice filter, but here coms few questions.

Should it follow:
a) javascript array slice arr.slice([begin[, end]])
b) php array_slice(int $offset [, int $length = NULL]) and string substr ( int $start [, int $length])
c) python has great slice(start, stop, [,step]) https://docs.python.org/2/library/functions.html#slice

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Jan 25, 2015

So, we should land some form of this simple but very often used filter. Reading through various issues / PRs opened for this topic I think that we've got few simple decisions to take:

  • - should we extend the existing limitTo filter or introduce a new one?
  • - if we introduce a new filter what should be it's name and API?
  • - if we introduce a new filter should we depreciate / remove the limitTo one?

My take would be to introduce a new slice filter with the API matching the JavaScript one as it:

  • does the job and is familiar to JS developers
  • works for both arrays and strings

Does anyone objects to the above plan? How do people feel about depreciating / removing the limitTo filter? I'm going to raise this during the team meeting tomorrow so it would be great to have feedback from the community.

@jlmagee
Copy link

@jlmagee jlmagee commented Jan 25, 2015

@pkozlowski-opensource Introduction of a new slice filter and leaving limitTo alone seems right to me

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Jan 25, 2015

@jlmagee the slight problem with leaving both limitTo and slice is that the impl of the 2 would be almost identical so we would be duplicating bytes for nothing. But maybe we can make one filter calling the other, to be seen.

@mgcrea
Copy link
Contributor

@mgcrea mgcrea commented Jan 25, 2015

Would love a new slice filter matching the JS API. I don't really like the idea of having redundant filters, so limitTo should ideally be deprectated. But there is so many examples out there using it that it might be best to leave it as it is, just maybe flag it as an alias in the docs.

But maybe we can make one filter calling the other

limitTo calling slice would be nice, but I'm wondering if it could have a performance impact.

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Jan 25, 2015

Thnx for the input @mgcrea. Yup, there might be slight perf impact (one more function call), but not sure if it would matter in practice. The other idea of making it easier on people:

  • depreciate limitTo in 1.4 and only remove in 1.5
  • remove in 1.4 and publish a compatibility module
@stryju
Copy link

@stryju stryju commented Jan 25, 2015

👍 for

remove in 1.4 and publish a compatibility module

@e-oz
Copy link

@e-oz e-oz commented Jan 25, 2015

removing limitTo is terrible idea! it's breaking change just because somebody likes how new word (slice) sounds - I can't believe you can be so irresponsible! Thousands of projects and plugins/libraries authors will be forced to spend thousands of man-hours just because mmm... you know... slice sounds more suitable for this case. You already can do filter with limit and offset and just for syntax sugar you want to add breaking change? I can't understand your motivation and not sure you understand consequences. Especially when you can just add second argument to existing filter.

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Jan 25, 2015

@jamm the current limitTo filter doesn't do what slice does. So it is not about finding a different name for the same thing. And hey, naming things is important 😄

BTW: it would be really great if we could focus on the technical merits, pros & cons and tune down the language a bit - calling people irresponsible in the very moment when we are reaching out to the community for feedback sounds odd...

@e-oz
Copy link

@e-oz e-oz commented Jan 25, 2015

@pkozlowski-opensource so add an argument and it will. If you want to add breaking change just because of naming - it's irresponsible. If you want to hear only positive feedback, without criticism - it's naive expectation. I'd glad to be less mean, but if we all will keep silence just to be "polite", breaking changes like this will ruin trust to Angular API at all.

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Jan 25, 2015

@jamm not sure why are saying that this change is only about a name... The crux of the "problem" is that:

  • limitTo filter only accepts one argument (limit)
  • proposed slice should accept 2 arguments (start and end index, as in JS)

So the 2 are functionally related but not the same. Could you please elaborate what you mean by:

You already can do filter with limit and offset and just for syntax

Are you referring to collection | limitTo:start | limitTo:-offset or something else?

@e-oz
Copy link

@e-oz e-oz commented Jan 25, 2015

@pkozlowski-opensource but if you will add second argument to limitTo filter, it will not break anything and will work. It's even proposed by author of this thread :)

Are you referring to collection | limitTo:start | limitTo:-offset

yes, but it's not so important. I agree limitTo:2:3 looks shorter and more handy, I'm only arguing about breaking change, nothing more.

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Jan 25, 2015

@jamm ok, cool, I think I got all your answers to my previous questions. Thnx for the feedback!

@Narretz
Copy link
Contributor

@Narretz Narretz commented Jan 26, 2015

So basically, if I use limitTo:2:3, then it behvaes like a normal slice filter? Sounds good to me. We could also alias slice to limitTo, althought that has no precedent.

tameraydin added a commit to tameraydin/angular.js that referenced this issue Jan 29, 2015
Extend the limitTo filter to take an optional argument for beginning index. It provides a slice-alike functionality to manipulate the input.

Closes angular#5355
tameraydin added a commit to tameraydin/angular.js that referenced this issue Jan 29, 2015
Extend the limitTo filter to take an optional argument for beginning index. It provides a slice-alike functionality to manipulate the input.

Closes angular#5355
@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Jan 29, 2015

Guys, we are leaning towards merging #10899 - if anyone is interested in this feature it is time to speak up now

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Feb 17, 2015

Woooohaaa! One of the most voted (and easiest to fix!) issues just got solved :-)

@revolunet
Copy link
Contributor

@revolunet revolunet commented Feb 17, 2015

👍

@tameraydin
Copy link
Contributor

@tameraydin tameraydin commented Feb 17, 2015

nice! it was my first contribution:)

@Hendrixer
Copy link
Contributor

@Hendrixer Hendrixer commented Mar 4, 2015

nice

@reissr
Copy link

@reissr reissr commented May 13, 2015

👍

netman92 added a commit to netman92/angular.js that referenced this issue Aug 8, 2015
Extend the limitTo filter to take an optional argument for beginning index.
It provides a slice-alike functionality to manipulate the input.

Closes angular#5355
Closes angular#10899
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.