Navigation Menu

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

lib: introduce forEach = flip map #64723

Merged
merged 9 commits into from Aug 18, 2019
Merged

Conversation

danbst
Copy link
Contributor

@danbst danbst commented Jul 14, 2019

lib: introduce forEach = flip map

The main purpose is to bring attention to flip map, which improves
code readablity. It is useful when ad-hoc anonymous function
grows two or more lines in map application:

    map (lcfg:
        let port = lcfg.port;
            portStr = if port != defaultPort then ":${toString port}" else "";
            scheme = if cfg.enableSSL then "https" else "http";
        in "${scheme}://cfg.hostName${portStr}"
    ) (getListen cfg);

Compare this to foreach-style:

    forEach (getListen cfg) (lcfg:
        let port = lcfg.port;
            portStr = if port != defaultPort then ":${toString port}" else "";
            scheme = if cfg.enableSSL then "https" else "http";
        in "${scheme}://cfg.hostName${portStr}"
    );

(the example was taken with modifications from apache-httpd module)

This is similar to Haskell's for (http://hackage.haskell.org/package/base-4.12.0.0/docs/Data-Traversable.html#v:for)

Extracted and simplified from a more radical approach in #57091.

The main purpose is to bring attention to `flip map`, which improves
code readablity. It is useful when ad-hoc anonymous function
grows two or more lines in `map` application:

```
      map (lcfg:
        let port = lcfg.port;
            portStr = if port != defaultPort then ":${toString port}" else "";
            scheme = if cfg.enableSSL then "https" else "http";
        in "${scheme}://cfg.hostName${portStr}"
      ) (getListen cfg);
```
Compare this to `foreach`-style:
```
      foreach (getListen cfg) (lcfg:
        let port = lcfg.port;
            portStr = if port != defaultPort then ":${toString port}" else "";
            scheme = if cfg.enableSSL then "https" else "http";
        in "${scheme}://cfg.hostName${portStr}"
      );
```
This is similar to Haskell's `for` (http://hackage.haskell.org/package/base-4.12.0.0/docs/Data-Traversable.html#v:for)
See `foreach`-introduction commit.
```
rg 'flip map ' --files-with-matches | xargs sed -i 's/flip map /foreach /g'
```
@danbst
Copy link
Contributor Author

danbst commented Jul 14, 2019

I can add release doc if this is going to be merged.

@danbst
Copy link
Contributor Author

danbst commented Jul 14, 2019

@volth I agree with your sarcasm! But OTOH I'd want flip map instead of map here:

https://github.com/NixOS/nixpkgs/blob/f3282c8/nixos/modules/services/web-servers/nginx/default.nix#L656

We can try to amend documentation, to teach people about flip map idiom from start. But we still have lots of non-functional people around, who are not used to reading flip-ed code.

@infinisil
Copy link
Member

How about just naming this for?

@danbst
Copy link
Contributor Author

danbst commented Jul 17, 2019

@infinisil I'd reserve for as a keyword for NixOS/nix#2543

@aanderse
Copy link
Member

aanderse commented Aug 5, 2019

@danbst & @volth consensus reached...?

@edolstra
Copy link
Member

edolstra commented Aug 5, 2019

This should be forEach since we use camelCase naming for functions.

See `forEach`-introduction commit.
```
rg 'flip map ' --files-with-matches | xargs sed -i 's/flip map /forEach /g'
```
@danbst danbst changed the title lib: introduce foreach = flip map lib: introduce forEach = flip map Aug 5, 2019
@danbst
Copy link
Contributor Author

danbst commented Aug 5, 2019

@aanderse @edolstra renamed.

Now forEach is called 10 times during

$ nix-instantiate ./nixos/ --arg configuration '{ boot.isContainer = true; }' -A system

@aanderse
Copy link
Member

aanderse commented Aug 18, 2019

@edolstra from your comments I assume implicit approval at this point?
@danbst can you fix merge conflicts and we merge?

@danbst danbst merged commit d09b4e3 into NixOS:master Aug 18, 2019
@danbst danbst deleted the flip-map-foreach branch August 18, 2019 15:48
@danbst
Copy link
Contributor Author

danbst commented Aug 18, 2019

thanks @aanderse and @edolstra

@aanderse aanderse mentioned this pull request Oct 18, 2019
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants