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
An API for load/edit/save .conf and .json files #272
Comments
@havocp @cprice404 So here's what I'm thinking: Step 1: We create a new Step 2: Create a new ConfigNode class. This will likely be an abstract class, with various subclasses representing certain types of values. These classes will contain the relevant tokens so that they can be modified or replaced as necessary. Child nodes will be stored in an Step 3: Write a new Parser. This Parser will be less complex than the current Parser. It will take all the tokens and construct an AST. This will allow us to find the relevant nodes when we call the In cases like the below:
We would NOT parse Step 4: Write the Step 5: Write the How does this sound? I'm thinking this would likely only support File at first. Also, when a path that hasn't been seen before is added, it should be rendered in the correct place by the tree traversal. I'm still unsure how we would handle the case in which the path appears more than once in the same file, though. |
Here's what I think the "separation of concerns" could be:
Re: the steps you mention, some thoughts:
As far as how to proceed - I think it might work best to avoid modifying the existing parser (parser2) at first, though you could borrow liberally from it; instead, focus on Parser1 and defining ConfigNode (if you like, make it a class AbstractConfigNode in the impl package, and we'll factor out a public config.ConfigNode interface later, using the same pattern as ConfigValue/AbstractConfigValue). You could make yourself a new test file ConfigNodeTest or whatever to test Parser1 and ConfigNode. If you start there, then we can write tests and work out what the kinds of ConfigNode are and generally know what's involved here. Reading through Parser.java as it exists, it looks quite hard to refactor to a Tokens=>ConfigNode parser, but maybe pretty easy to refactor so that it parses ConfigNode instead of parsing Tokens. That is, most of the "shape" of Parser.java looks to me like it's driven by the needs of generating ConfigValue. I could be wrong as always. Remember that the file could be considered either JSON or HOCON, and if it's JSON we need to enforce JSON rules and also ensure we create valid JSON. A thorny issue for ConfigNode/ConfigDocument, perhaps, is how to arrange a document like this so it's easy to edit a value using the API:
So from the standpoint of recreating this file, you probably need a tree that matches how it's written above... a node However, that syntax-retaining tree will be annoying from an editing standpoint, and the
This sounds more terrible than it is, since in practice values and files you edit by machine probably won't be that complicated. So a simple rule like "remove all nodes but the last one and change the last one's value" is likely fine. That could become a method like Implementation-wise, remember that we use an all-immutable convention. For trees, one implication of this is that nodes can't have a |
@havocp Hmm, alright, that makes sense. So it sounds like maybe I should start by defining a In terms of inserting into the tree, I'm thinking we should leave all the nodes but change the value of the last one to |
For our use cases, I think we pretty much frown upon (or just outright don't support) cases where someone defines the same value multiple times in the same file. So from our perspective I'd love to see us end up in a state where modifying a value would effectively remove dupes from the file... but assuming we're not the only target audience for these changes :), we'd obviously be happy to defer to whatever algorithm you think is best, @havocp . |
I think the document.replaceAll kind of method will effectively just be a convenience method - you could also iterate over the nodes by hand and do what you want - but we should try to make it conveniently do what we think people will want. I think I agree with removing dups and changing the last one. Starting with parser1/ConfigNode and a test file for same sounds good to me. |
@havocp Here's what I'm thinking for the There will be an abstract
Does this sound sane/like what you were thinking? Also, @cprice404 you might be interested in this |
The |
It sounds reasonable to me; I would mostly have naming nitpicks I think. I guess I'd also say, try it and maybe we'll have to change it after we see how it goes, but it sounds like a good first thing to try. Naming thoughts (not the final word, just ideas):
Don't get too hung up on the names; we can easily search-and-replace later. The main reason I bring it up is that I think naming can be helpful for conceptual clarity. Mostly I'd say just give it a shot and see how it turns out, it's hard for me anyway to predict this too much a priori. |
@havocp So I'm getting to work now on indentation when adding to a This has two phases: indentation calculation and indentation addition to a value node. To calculate the indentation, we can loop through the list of children in the object, and check for newline characters. If there is no newline character, we add the new setting onto the same line. If there IS a newline character, we calculate the indentation on the last line of the object, and use that indentation, adding the setting on its own line. We will then have to add the indentation to a value node. This should not be particularly difficult; we can have a new protected method on The only problem I can see with this approach is the case in which we're inserting a multi-line value into a single-line object. So in a case like this:
at path Mostly I just wanted to run this approach by you, and see if you had any suggestions/feedback, or if you'd prefer a different approach. Thanks! |
Philosophically I think this can be a pretty simple best-effort algorithm - if the file is nicely-formatted already in a reasonable way, then we try to match that, but if the file is too creative then too bad. As long as we produce a valid file it's ok to be a little ugly in corner cases. For I don't mean to imply you'd reformat the existing stuff; just if you add fields inside Anyway I think what you describe sounds fine. Let's just do something simple that works in common cases (and always produces a valid file, even in the corner cases where it gets the indentation a little weird). |
Sounds good! Thanks for the feedback as always! |
I think we can declare victory on this issue and close it! There are some more APIs we want to do, but I'll make new issues for those (or if I don't, anyone is welcome to of course). Thanks so much for the work on this. |
Continuing the discussion from #149 , once we merge #271 we will have all of the information we need to save the file again in memory.
Some initial thoughts...
The currently-private API in https://github.com/typesafehub/config/blob/master/config/src/main/java/com/typesafe/config/impl/Tokens.java is pretty awful as a public API, with no real thought to clarity, namespacing, extensibility, or anything of that nature.
I also recently messed with SimpleConfigOrigin in #267 and discovered that it's quite a can of worms to change that thing. Not impossible but not much fun either. I'm also a little concerned about the memory footprint of hanging a bunch of tokens off of every ConfigOrigin, when most apps won't use the info at all. (A potential solution would be to drop the tokens at the first use of
withFallback
.)But, I'm not sure we even need the tokens on the origin, because if we have a
ConfigDocument
kind of API, it could simply support getting the nodes at a given path.What I don't know yet is what the
ConfigDocument
orConfigNode
API should look like, exactly.It could be very limited, supporting only setting paths to values, parsing the doc into a config, and rendering:
Questions already arise:
ConfigDocument
, maybe we only support parsing Reader and File?Still, a minimal API like that is potentially good enough for load/edit/save ? If so we can go with YAGNI and not do a lot more to start ...
The thing I'm wondering whether we need, would be a
ConfigNode
kind of thing - perhaps slightly higher-level than tokens, or perhaps almost the same as our current set of tokens, I'm not sure - this would allow looking at substitution expressions, comments, and other stuff and manipulating them sort of like the DOM. This may be needed for something like #225, but it's a lot more API surface area to get wrong.You would also need an API like this to write a HOCON formatter, for example.
It could be worth tackling the basic "load, change some values, save" use-case before the general AST use-cases, but I don't know.
The text was updated successfully, but these errors were encountered: