X Tutup
Skip to content

feat(router): angular1 router#3698

Closed
btford wants to merge 3 commits intoangular:masterfrom
btford:shahata-btford-feat-angular1-router
Closed

feat(router): angular1 router#3698
btford wants to merge 3 commits intoangular:masterfrom
btford:shahata-btford-feat-angular1-router

Conversation

@btford
Copy link
Copy Markdown
Contributor

@btford btford commented Aug 18, 2015

Supersedes #3138

I did a bit of cleanup on @shahata's PR. I'm going to let CI for this run, and push it to presubmit if it passes.

@IgorMinar is going to review this, but we should get this in before it gets bitrotten again.

@btford btford added feature Label used to distinguish feature request from other issues comp: router labels Aug 18, 2015
@btford btford added this to the alpha-36 milestone Aug 18, 2015
@googlebot
Copy link
Copy Markdown

googlebot commented Aug 18, 2015

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@btford btford mentioned this pull request Aug 18, 2015
Closed
gkalpak
gkalpak reviewed Aug 18, 2015
View reviewed changes
modules/angular1_router/lib/facades.es5 Outdated
Copy link
Copy Markdown
Member

@gkalpak gkalpak Aug 18, 2015

Choose a reason for hiding this comment

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

@btford, continuing the discussion from #3138 (comment), get(), contains() and isEmpty() should use hasOwnProperty() (unless the objects passed to them have no prototype).

Even facade/collection.ts uses hasOwnProperty().

E.g. isEmpty() will never return true, if there are properties/functions on the prototype.

Copy link
Copy Markdown
Contributor Author

@btford btford Aug 18, 2015

Choose a reason for hiding this comment

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

Ah – this is legit. 👍

@btford btford added the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 18, 2015
IgorMinar
IgorMinar reviewed Aug 18, 2015
View reviewed changes
modules/angular1_router/test/ng_outlet_spec.js
Copy link
Copy Markdown
Contributor

@IgorMinar IgorMinar Aug 18, 2015

Choose a reason for hiding this comment

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

xit-ed test - is this still correct? the issue listed above is in the wrong repo

Copy link
Copy Markdown
Contributor Author

@btford btford Aug 18, 2015

Choose a reason for hiding this comment

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

yes– this behavior needs to be re-thought regardless.

@btford btford force-pushed the shahata-btford-feat-angular1-router branch from aa05af5 to e5a711d Compare August 18, 2015 23:34
@btford
Copy link
Copy Markdown
Contributor Author

btford commented Aug 18, 2015

I've removed all but one xit.

I'm hoping we can get this in today and then I'll spend the rest of the week improving tests and refactoring.

@btford
Copy link
Copy Markdown
Contributor Author

btford commented Aug 19, 2015

@IgorMinar gave me the LGTM to merge this with the understanding that I'll follow up with some refactor and cleanup work.

Pushing to presubmit 🐈

@shahata shahata closed this in ddb62fe Aug 19, 2015
@vicb vicb reopened this Aug 20, 2015
@vicb
Copy link
Copy Markdown
Contributor

vicb commented Aug 20, 2015

@btford I'm re-opening this PR as it had to be reverted (Travis was red)

@0x-r4bbit 0x-r4bbit mentioned this pull request Aug 20, 2015
Closed
@frodefi frodefi mentioned this pull request Aug 20, 2015
Open
@vicb vicb mentioned this pull request Aug 20, 2015
Closed
@btford
Copy link
Copy Markdown
Contributor Author

btford commented Aug 20, 2015

This PR clearly was not the cause of Travis going red. After reverting this commit, we still see the same failure.

@vicb can you re-merge this please?

@btford btford removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 20, 2015
@btford
Copy link
Copy Markdown
Contributor Author

btford commented Aug 20, 2015

Reverted @vicb's revert. This change had nothing to do with breakage in master. This is back in master.

@vicb – next time please make sure CI passes before pushing to changes to master, even if master is broken.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@IgorMinar IgorMinar

Labels

cla: no feature Label used to distinguish feature request from other issues

Projects

None yet

Milestone

alpha-36

Development

Successfully merging this pull request may close these issues.

6 participants

@btford @googlebot @vicb @IgorMinar @gkalpak @mhevery
X Tutup