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

add Array.prototype.filter signature with type guard #10916

Conversation

maiermic
Copy link

10027 also didn't add the overload to Array#filter, only to ReadOnlyArray#filter. @mhegazy could you reopen this issue?

-- Arnavion's comment

This only adds it to ReadOnlyArray though. Also need it for regular Array.
-- Arnavion's comment

See #7657 and #10027

@msftclas
Copy link

Hi @maiermic, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@mhegazy
Copy link
Contributor

mhegazy commented Sep 14, 2016

thanks @maiermic, some baslines changed, you will need to do a gulp baseline-accept

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

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

We will need to update the baselines as well. gulp baseline-accept will do the trick.

@maiermic
Copy link
Author

@mhegazy Thanks, I appreciate your help.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 14, 2016

waiting on the CI.

@maiermic
Copy link
Author

maiermic commented Sep 14, 2016

@mhegazy Two tests failed on my machine. I ran gulp baseline-accept a second time and it modified two more files. I guess all tests will pass now, but this time I wait until my local test runner is done.

Update: Tests passed on my machine and I pushed my changes. Waiting on the CI again 😅

@bcherny
Copy link

bcherny commented Dec 7, 2016

@mhegazy Is it possible to merge this?

@mhegazy
Copy link
Contributor

mhegazy commented Dec 23, 2016

Sorry this fell off my list for a while. one more comment, we need to update the filter method on ReadOnlyArray, and the TypedArray varients, e.g. Int32Array (all in the same file).

@maiermic
Copy link
Author

@mhegazy This pull request only updates filter on Array. I updated filter on ReadOnlyArray in #10027 (already merged). TypedArray varients cann't use a type guard. They are not generic and contain always the same type. No update possible. So it should be possible to merge this.

@ikokostya
Copy link
Contributor

@mhegazy Could you review again taking into account latest comment #10916 (comment)?

@teppeis
Copy link

teppeis commented May 2, 2017

@maiermic @mhegazy any updates? I could resolve the conflicts and create new pr if you need.

Michael Maier added 2 commits May 4, 2017 07:02
# Conflicts:
#	tests/baselines/reference/arrayFilter.symbols
#	tests/baselines/reference/arrayFilter.types
#	tests/baselines/reference/declarationEmitPromise.symbols
#	tests/baselines/reference/declarationEmitPromise.types
#	tests/baselines/reference/genericMethodOverspecialization.symbols
#	tests/baselines/reference/genericMethodOverspecialization.types
@maiermic
Copy link
Author

maiermic commented May 4, 2017

@teppeis Conflicts are resolved by my latest update. I'm waiting for @mhegazy to merge.

@NaridaL
Copy link
Contributor

NaridaL commented Jun 9, 2017

What's the status on this?

@mysticatea
Copy link

@mhegazy @maiermic What is happening with this?

@maiermic
Copy link
Author

This has been solved by #16223 in the meantime.

@maiermic maiermic closed this Jul 17, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants