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

Functions defined in let bindings #61

Closed
stil4m opened this issue Mar 19, 2017 · 5 comments
Closed

Functions defined in let bindings #61

stil4m opened this issue Mar 19, 2017 · 5 comments
Milestone

Comments

@stil4m
Copy link
Owner

stil4m commented Mar 19, 2017

let are commonly used to hold intermediate values that are used (multiple times) in other expressions in the let binding.

There is no reason to have functions in there, extracting these to top-levels would be a better choice. Encapsulation can be achieved by not exposing that extracted function or by creating a whole new module.

@stil4m stil4m added this to the 0.6.0 milestone Mar 19, 2017
@eeue56 eeue56 mentioned this issue Mar 19, 2017
14 tasks
@felixLam
Copy link
Contributor

I don't agree that this is an absolute. We have a few instances where there is a generic top level function and some othe functions calls this multiple times with only one differing argument. We then create a local alias that encapsulates the call along with the common arguments used in that function.

@felixLam
Copy link
Contributor

But I can always switch that check off.

@stil4m
Copy link
Owner Author

stil4m commented Mar 19, 2017

I think there should be a distinction between curried function that become new functions and actually defining new functions.
In the following example I believe you do not want bar but foo is ok even though they are semantically equivalent.

foo x = 
   let 
     listModifier = List.map ((+) 1)
   in
     listModifier x

bar x = 
    let 
       incrementList y = List.map ((+) 1) y
    in
       incrementList x

@felixLam
Copy link
Contributor

felixLam commented Mar 19, 2017 via email

stil4m pushed a commit that referenced this issue Mar 29, 2017
@stil4m
Copy link
Owner Author

stil4m commented Mar 29, 2017

This check will be disabled by default for now.

@stil4m stil4m closed this as completed Mar 29, 2017
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

No branches or pull requests

2 participants