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

Remove unnecessary limit check #5043

Merged
merged 1 commit into from
Jan 16, 2018
Merged

Conversation

MajorBreakfast
Copy link
Contributor

@MajorBreakfast MajorBreakfast commented Jan 15, 2018

https://github.com/Polymer/polymer/blob/master/lib/elements/dom-repeat.html#L555-L556

const limit = Math.min(isntIdxToItemsIdx.length, this.__limit);
      for (; instIdx<limit; instIdx++) {

So, for each interation of the loop this holds:

instIdx < Math.min(isntIdxToItemsIdx.length, this.__limit)

Which implies:

instIdx < this.__limit

Consequently && instIdx < this.__limit is essentially the same as && true and can be left out.

@MajorBreakfast
Copy link
Contributor Author

I stumbled over this because the else branch creates new instances. It makes no sense to create new instances when the limit is exceeded. But, the check is redundant and thus this never happens.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Tests are passing in Firefox and since this is a logical change, it should be okay in Chrome as well. The change SFTM, but I would like to have full approval by @kevinpschaaf or @sorvell as they are more knowledgeable about this implementation.

@MajorBreakfast
Copy link
Contributor Author

@TimvdLippe I (and, it seems, Google also) have never heard "SFTM" before :) What does it mean? :D

I think the CI is currently a bit broken: "Error response status: 6 Selenium error: no such session"

@TimvdLippe
Copy link
Contributor

@MajorBreakfast SFTM means Seems Fine To Me. It basically means "I can't spot anything wrong, but I can't say for certain it is correct" Basically someone else might be able to spot a problem with it, because that person is more knowledgeable about the component.

CI has been spotty for a while now, still trying to figure out what is going on.

@MajorBreakfast
Copy link
Contributor Author

Ah, I figured that it must mean something along those lines :)

Copy link
Member

@kevinpschaaf kevinpschaaf left a comment

Choose a reason for hiding this comment

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

LGTM, checked the history and that conditional seems to have been a holdover from before the local limit was used as the loop terminal (https://github.com/polymer/polymer/blame/3dd5593e9bbdbaa241593852d2ffa46e50040319/src/templatizer/dom-repeat.html#L490). Agree it's redundant.

@TimvdLippe TimvdLippe merged commit 1215f8d into Polymer:master Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants