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

Use exists instead of filter + match on [] #1212

Merged
merged 1 commit into from May 21, 2016
Merged

Conversation

forki
Copy link
Contributor

@forki forki commented May 21, 2016

No description provided.

match others |> List.filter (function (TyparConstraint.SupportsNull(_)) -> not (TypeSatisfiesNullConstraint cenv.g m cxty) | _ -> true) with
| [] -> Some cxty
| _ -> None
if others |> List.exists (function (TyparConstraint.SupportsNull(_)) -> not (TypeSatisfiesNullConstraint cenv.g m cxty) | _ -> true)
Copy link
Contributor

Choose a reason for hiding this comment

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

would we prefer this style:

// Throw away null constraints if they are implied 
let hasTypesWithNullConstraint =
    others 
    |> List.exists (
        function
        | (TyparConstraint.SupportsNull(_)) -> not (TypeSatisfiesNullConstraint cenv.g m cxty) 
        | _ -> true
    ) 
if hasTypesWithNullConstraint 
then None
else Some cxty

I know most of the compiler has long lines, but from my perspective it really ease the flow of reading when we adopt shorter lines / expressions, introducing / naming a variable also helps

Copy link
Contributor

Choose a reason for hiding this comment

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

@smoothdeveloper Per CONTRIBUTING,md, that sort of thing would be a separate cleanup PR (though it's fair to say that reducing line length isn't really a cleanup aim at the moment)

@dsyme
Copy link
Contributor

dsyme commented May 21, 2016

This is very simple, merging now

@dsyme dsyme merged commit eece704 into dotnet:master May 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants