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 support for sentinel and length refinement of tuples #5048

Closed
wants to merge 2 commits into from

Conversation

popham
Copy link
Contributor

@popham popham commented Oct 6, 2017

Fixes #4050, fixes #4377, fixes #4556, and fixes #4873.

@popham popham force-pushed the tuple-refine branch 3 times, most recently from 4ae4132 to dfbef6f Compare October 6, 2017 23:30
@bradennapier
Copy link

+1 ! Definitely need this!! :-)

@anru
Copy link

anru commented Mar 14, 2018

it's absolutely killer feature for flow.

@vkurchatkin @mroch could we get merged this PR/functionality into the codebase ?

@petejodo
Copy link

I'd like to use this for better refining React children. To give a semi-contrived example:

type LayoutOneProps = {
    layoutOne: boolean,
    children: [
        React.Element<typeof Level>,
        React.Element<typeof Sidebar>,
        React.Element<typeof Content>
    ],
};

type LayoutTwoProps = {
    layoutTwo: boolean,
    // currently required to have `null` as the last value -------------------------v
    children: [React.Element<typeof LeftColumn>, React.Element<typeof RightColumn>, null],
};

const LayoutWrapper = (props: LayoutOneProps | LayoutTwoProps) => {
    if (props.layoutOne) {
        return (
            <div className="container">
                <div className="row">
                    {props.children[0]}
                </div>
                <div className="row">
                    {props.children[1]}
                    {props.children[2]}
                </div>
            </div>
        );
    } else if (props.layoutTwo) {
        return (
            <div className="container">
                <div className="row">
                    {props.children}
                </div>
            </div>
        );
    }
};

<LayoutWrapper layoutTwo>
    <LeftColumn />
    <RightColumn />
    {null /* currently required to do this */}
</LayoutWrapper>

@bradennapier
Copy link

Not sure why no one from the Flow team reviews or accepts any PR's... it kind of sucks... this is a year old and still flow fails at any kind of tuple refinement.

(* The `raw` string is no good as a refinement key because of equivalence
classes like "1" = 1 = 0.9... = 1.0 = .... We just leave 0.9... to OCaml
randomness because the ES spec leaves its handling ambiguous. *)
let name =
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably use Dtoa.ecma_string_of_float

@goodmind
Copy link
Contributor

Hey @mroch. Maybe you will find this useful too for tuples

@goodmind goodmind added the Stalled Issues and PRs that are stalled. label Jul 27, 2019
@ilya-bobyr
Copy link
Contributor

Rebased this change on top of master in #7974

ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Sep 4, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Sep 4, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Sep 6, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Sep 8, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Sep 8, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Sep 15, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Sep 16, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Sep 16, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Sep 30, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Oct 4, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
ilya-bobyr added a commit to ilya-bobyr/flow that referenced this pull request Oct 10, 2019
Based on facebook#5048

Co-authored-by: Tim Popham <[email protected]>
Co-authored-by: Ilya Bobyr <[email protected]>
facebook-github-bot pushed a commit that referenced this pull request Oct 10, 2019
Summary:
I have rebased changes from #5048 on top of the current master branch. (It was ~4000 changes behind %)
Addressed the only comment that the review had.

I must admit that I do not understand the flow source code as well as the original author.
But I will do my best to make sure this gets in.

I can see the original pull request was marked "interesting" here: https://github.com/facebook/flow/projects/4#card-20788575

It also feels that the test coverage is incomplete.  There are no tests that cover error messages.
Only the happy path is tested.  I did not figure out yet how to add this kind of tests.  Any kind of guidance is very welcome.
Pull Request resolved: #7974

Reviewed By: jbrown215

Differential Revision: D16902858

Pulled By: panagosg7

fbshipit-source-id: 54c0cc341d50d95ce730d4c50f3e37e458bee192
@STRML
Copy link
Contributor

STRML commented Jan 25, 2021

It's a shame to see great PRs like this get left unreviewed and unmerged - this is a hugely useful feature.

@ilya-bobyr
Copy link
Contributor

It's a shame to see great PRs like this get left unreviewed and unmerged - this is a hugely useful feature.

We got the "length" part in, in this PR: #7974

I have rebased the original change to work in compile codebase as of about a year ago.
But as @goodmind pointed out in the discussion for PR 7974, a technical discussion might need to happen before a final implementation would go through a review.

@STRML
Copy link
Contributor

STRML commented Jan 26, 2021

Thanks for pointing that out. Length refinement is rather limited but workable. Truthiness checks and less than / greater than does not trigger the refinement.

See this example.

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in c0401c3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Merged Stalled Issues and PRs that are stalled. Typing: tuple
Projects
None yet
8 participants