lib: add type signature to some of the functions#495873
lib: add type signature to some of the functions#495873ilkecan wants to merge 3 commits intoNixOS:masterfrom
Conversation
There was a problem hiding this comment.
Thanks for engineering all the types. I know its a pain. So thanks for doing it :)
I would like to note some minor things for style consistency:
We should probably add some better styleguide if not already described in the doc/contributing
- Section ordering: Examples before Types:
- Examples are concrete, types are abstract: readers understand a concrete usage example faster than a type signature.
- Type variables:
- Concrete types start with uppercase: Int, String, Map, Bool, Derivation
- Type variables start with lowercase: a, b, k, v, m
- If we use
Mapshould we also useListinstead of[]?
EDIT:
Made a PR for the ordering:
lib/attrsets.nix
Outdated
|
|
||
| ``` | ||
| attrsets.longestValidPathPrefix :: [String] -> Value -> [String] | ||
| longestValidPathPrefix :: [String] -> Value -> [String] |
There was a problem hiding this comment.
| longestValidPathPrefix :: [String] -> Value -> [String] | |
| longestValidPathPrefix :: [String] -> Any -> [String] |
|
|
||
| : Value to trace | ||
|
|
||
| # Type |
There was a problem hiding this comment.
Can you switch the order of "Examples" vs "Type" ?
lib/derivations.nix
Outdated
| # Type | ||
| ``` | ||
| warnOnInstantiate :: string -> Derivation -> Derivation |
There was a problem hiding this comment.
| warnOnInstantiate :: string -> Derivation -> Derivation | |
| warnOnInstantiate :: String -> Derivation -> Derivation |
lib/filesystem.nix
Outdated
|
|
||
| ``` | ||
| packagesFromDirectoryRecursive :: { | ||
| callPackage :: Path -> {} -> a, |
There was a problem hiding this comment.
| callPackage :: Path -> {} -> a, | |
| callPackage :: Path -> AttrSet -> Derivation, |
To make it consistent with the rest. Otherwise the {} could be interpreted as the empty set.
lib/meta.nix
Outdated
| # Type | ||
|
|
||
| ``` | ||
| appendToName :: string -> Derivation -> Derivation |
There was a problem hiding this comment.
| appendToName :: string -> Derivation -> Derivation | |
| appendToName :: String -> Derivation -> Derivation |
Use capital String please. To avoid potential confusion with types.string
I think all named types should be capitalized for consistency.
lib/meta.nix
Outdated
| # Type | ||
|
|
||
| ``` | ||
| lowPrioSet :: (Map string Derivation) -> (Map string Derivation) |
There was a problem hiding this comment.
I am not a big fan of borrowing more from haskell.
In haskell:
stringshould beString: Type names in Haskell must start with an uppercase letter. Lowercase string would be interpreted as a type variable.- Parentheses are unnecessary: While not wrong, the parens around Map String Derivation are redundant here.
Idiomatic Haskell:
lowPrioSet :: Map String Derivation -> Map String Derivation If string is intentionally a type variable (i.e., polymorphic over the key type), then it would be valid but you'd
typically use a or k by convention:
lowPrioSet :: Map k Derivation -> Map k Derivation I would look into some other files in lib first to see if there is convention in place that we can borrow instead.
lib/trivial.nix
Outdated
| # Type | ||
|
|
||
| ``` | ||
| and :: bool -> bool -> bool |
There was a problem hiding this comment.
| and :: bool -> bool -> bool | |
| and :: Bool -> Bool -> Bool |
lib/trivial.nix
Outdated
| # Type | ||
|
|
||
| ``` | ||
| compare :: a -> a -> int |
There was a problem hiding this comment.
| compare :: a -> a -> int | |
| compare :: a -> a -> Int |
lib/trivial.nix
Outdated
| # Type | ||
|
|
||
| ``` | ||
| toHexString :: int -> string |
There was a problem hiding this comment.
| toHexString :: int -> string | |
| toHexString :: Int -> String |
|
Holding a PR up with a style rule that wasn't there before is kind of bogus. I think this is fine to merge as-is with the select edits about the types themselves. |
Majority of the existing lib functions use the ordering:
which is why I have ordered the new type signatures that way and also "fixed" some of the outliers. Of course current situation should not be an obstacle for possible improvements, but I think this deserves a separate discussion and existing docs should be updated afterwards depending on #495918.
Yeah, that makes sense to me.
I initially saw for the builtin functions. But then saw existing But upon further look, I think nixpkgs also uses |
- concrete types start with uppercase: Int, String, Bool, Derivation,
etc.
- type variables start with lowercase: a, b, etc.
- list:
- use `[x]` for homogeneous lists instead of `List x` or `[ x ]`
- use `List` for heterogeneous lists (not that common in `lib`)
- attr:
- use `AttrSet` for a generic attribute set type
- use `{ key1 :: Type1; key2 :: Type2; ... }` adding signatures for
known attribute names and types
- end with an ellipsis (`...`) if the set can contain unknown
attributes
- use `{ [String] :: x }` if all the attributes has the same type `x`
- prefer `Any` over `a` if the latter is not reused
I was looking for the list difference function using noogle and couldn't find
subtractListsinitially, because it lacks a type signature.I tried to add type signatures to most of the non-module or non-internal functions, as best as I could.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.