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

Added functions and types for generating config file #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Added functions and types for generating config file #14

wants to merge 4 commits into from

Conversation

fluffynukeit
Copy link

I added some functions and types for generating a config file. Below is the sample output from the a new writeTest:

# This is a test comment
# And here is a new one

import "pathological.cfg"
myBindVar = "here's my bind string"
myInt = 32 # This is a trailing comment as part of a bind
# Ok, let's try a group now
group1 {
  # Starting group 1
  group1a = 7
  group1b = 90
  group2 {
    # I am nested!
    import "import.cfg"
    group2a = "nested var"
  }
  # End the group
}

# A couple more binds for good measure
penultimate = "almost"
final = "there"

It's pretty rudimentary but I expect it to flexible enough (if not convenient) to handle most use cases. The default tab size of two spaces is not currently configurable.

@bos
Copy link
Owner

bos commented Apr 24, 2013

Thanks for your patch. I'm afraid it has a number of big difficulties that make it a non-starter in its current form.

I would have expected the API to be based around the existing Directive type, or to have at least some kind of sensible correspondence to it, but the new FileEntry type is unrelated, and more importantly it is not type-safe.

There's no obvious way to round-trip a config through this API, i.e. parse a file, modify something (or nothing), and encode it back out.

You should probably be using a pretty printing library to generate nicely indented output, rather than your current setup of tracking indentation by hand.

The strategy of pasting together a big string will have very poor performance if someone wants to write a large config file (there is a 'Builder' type for exactly this purpose).

I don't see the point of all of the top-level functions you have added. They almost double the size of the API without making client code any more readable or expressive.

I appreciate you submitting a patch that adds tests. That is a good thing; thank you.

Long story short, I'd like to see this capability added in a way that is expressive and elegant. I have confidence that you can get there, but this patch doesn't yet have either property.

@fluffynukeit
Copy link
Author

You are definitely right. All of those criticism are right on the money and hardly surprising to hear. I originally wanted to leverage the existing Directive type, but could not think of a way to get comments and newlines within the Group record. I also wanted to keep currently concealed types hidden. I found the Value type added complexity when trying to go from config to file, instead of the other way around which seems more natural. In the end, I modeled my patch after my earlier self-rolled solution, which if you can believe is even more rudimentary.

I appreciate your words of encouragement, and I might revisit this in the future when I feel I have enough skill to tackle it. My exploration of Haskell is, however, largely guided by my needs in developing my hobby project. My self-rolled solution for writing a config is adequate for those purposes, so I plan to focus on that for the time being.

If you choose to pursue this yourself, or get a patch with a solution you are happy with, I would certainly find the code to be educational.

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.

2 participants