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/generators: add toKeyValue & mkKeyValueLine #20903

Merged
merged 1 commit into from
Dec 4, 2016

Conversation

Profpatsch
Copy link
Member

generators for the common use case of simple config files which hold keys and
values. Used in the implementation for toINI.

  • three new unit tests that pass, old tests pass as well

Can be used in #20888 directly.

@mention-bot
Copy link

@Profpatsch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @MarcWeber and @zagy to be potential reviewers.

generators for the common use case of simple config files which hold keys and
values. Used in the implementation for toINI.
@Profpatsch Profpatsch changed the title lib/genators: add toKeyValue & mkKeyValueLine lib/generators: add toKeyValue & mkKeyValueLine Dec 4, 2016
Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

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

Seems OK, at a superficial look.

@Profpatsch Profpatsch merged commit ea412cd into NixOS:master Dec 4, 2016
@edolstra
Copy link
Member

edolstra commented Dec 5, 2016

Can't this be renamed to mkIniFile or something like that? ...KeyValue says absolutely nothing.

@Profpatsch
Copy link
Member Author

Can't this be renamed to mkIniFile or something like that?

Ah, it creates a key-value config file, toINI generates the ini files.
Hm, but not sure what a better name for that would be; maybe improve the docs?

The generators themselves are named toX, the exported helper functions mkX. All generators have the same calling convention (first argument configuration, second the data). Maybe the helpers should use a different naming scheme?

@vcunat
Copy link
Member

vcunat commented Dec 5, 2016

mkKeyValueLine function is very general, so it seems hard to come with a more concrete name. If it's unlikely to be used from a different file, we might hide it in a let at the top.

@Profpatsch
Copy link
Member Author

If it's unlikely to be used from a different file, we might hide it in a let at the top.

Well, it’s used in the default implementation of the mkKeyValue config options, mkKeyValueLine "=". On second thought, it should probably be named mkKeyValueDefault instead, since Line sounds like it ends with \n .

@Profpatsch
Copy link
Member Author

I have renamed the function in #20920

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