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.trivial: add a new importJSON function #13552

Merged
merged 2 commits into from Feb 29, 2016
Merged

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Feb 28, 2016

Things done:
  • Tested using sandboxing (nix-build --option build-use-chroot true or nix.useChroot on NixOS)
  • Built on platform(s): NixOS / OSX / Linux
  • Fits CONTRIBUTING.md.

This is meant to be used by packages who often re-generate their outputs.

Producing valid JSON is easier than nix, and also garantees it's purity.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra, @nbp and @shlevy to be potential reviewers

@shlevy
Copy link
Member

shlevy commented Feb 28, 2016

Why is this in lib.modules?

@vcunat
Copy link
Member

vcunat commented Feb 28, 2016

(Off-topic.) Hmm, it's a shame that the nix language doesn't have the composition operator. I've seen quite a lot situations where I would use it. Here it doesn't simplify much, builtins.fromJSON . builtins.readFile, but when the nesting is deep, things get ugly without a similar operator.

@zimbatm
Copy link
Member Author

zimbatm commented Feb 28, 2016

@shlevy because I didn't know where to put it, happy to move

@vcunat totally thought about that during the coding of this

@shlevy
Copy link
Member

shlevy commented Feb 28, 2016

Probably trivial.nix

@zimbatm zimbatm changed the title lib.modules: add a new importJSON function lib.trivial: add a new importJSON function Feb 28, 2016
@zimbatm
Copy link
Member Author

zimbatm commented Feb 28, 2016

@shlevy fixed

This is meant to be used by packages who often re-generate their inputs.

Producing valid JSON is easier than nix, and also garantees it's purity.
@domenkozar
Copy link
Member

Let's also replace
pkgs/build-support/build-maven.nix: info = builtins.fromJSON (builtins.readFile infoFile);
and then merge.

@zimbatm
Copy link
Member Author

zimbatm commented Feb 29, 2016

@domenkozar done. I don't see any users of buildMaven ?

domenkozar added a commit that referenced this pull request Feb 29, 2016
lib.trivial: add a new importJSON function
@domenkozar domenkozar merged commit 05cadcb into NixOS:master Feb 29, 2016
@zimbatm zimbatm deleted the import-json branch February 29, 2016 13:57
@Profpatsch
Copy link
Member

(OT) @vcunat @zimbatm Was missing this, too. Even having compose = f: g: x: f (g x) in lib/triivial would be a great step forward.
I also thought about a composeN function that takes a list of functions and composes them all from right to left (since without operator it quickly degrades to (compose (compose (compose …)))

@vcunat
Copy link
Member

vcunat commented Apr 4, 2016

Off-topic. TL;DR: composition of functions is a nice thing, but currently I can't see enough use cases in nixpkgs to be worth of action. (If it was, I would open an issue, but this way I didn't know where to post instead.)

Composition is associative and it's useful in some programming styles (e.g. in shells). We can't have a similar (infix) operator without changing nix, I think, but we could use lists, as you suggest:

let # to fix highlighting
  applyList = fold (f: g: x: f (g x)) (x: x);
  # so instead of haskell's: fun call 1 . fun call 2 . fun call 3 $ point
  # we might use             applyList [ (fun call 1) (fun call 2) (fun call 3) ] point

# that would give
  importJSON = applyList [ builtins.fromJSON builtins.readFile ];
# instead of
  importJSON = path: builtins.fromJSON (builtins.readFile path);

# maybe better use case, but a completely singular one
  top-level = fix' ( applyList (map extends [ customOverrides stdenvOverrides /*...*/ ]) (self: {}) )
  # ^ it's likely I've got some bug on this line

@Profpatsch
Copy link
Member

+1, but I’d like to have the base (and conceptually nicer) case as well and then define the more specific function with it (fold over compose)

And of course there is a naming question. I think compose is a pretty well-known name, so I’m not sure if it’s a good idea to call the list function applyList. composeList? composeL? I chose composeN above because the function composes n functions.

@vcunat
Copy link
Member

vcunat commented Apr 4, 2016

I haven't thought much about the name, as I could find almost no places to use it.

@vcunat
Copy link
Member

vcunat commented Apr 9, 2016

I also thought about a composeN function that takes a list of functions and composes them all from right to left (since without operator it quickly degrades to (compose (compose (compose …)))

Oh, I've been blind! You can nicely avoid nesting parentheses even with this binary compose:

compose f1 compose f2 compose f3 f4 == f1 . f2 . f3 . f4 # NOT true

You can even easily append the final application.

@nbp
Copy link
Member

nbp commented Apr 9, 2016

Another option would be to have something like:

{ composeFun = f: g:
    if isFunction g then
      y: composeFun (x: g (f x)) y
    else
      f;

  importJSON = x: assert !(isFunction x);
    composeFun builtins.readFile builtins.fromJSON x;

Unfortunately, this is not as nice as the composeList approach, as this one leaks the composeFun recursion, thus we have to assert and check, which adds strictness.

@vcunat
Copy link
Member

vcunat commented Apr 9, 2016

My last post doesn't hold. I misplaced one parenthesis.

@Profpatsch
Copy link
Member

if isFunction g then

No, please no. Just no.

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

7 participants