[Addition] Added IniRenderer and IniRenderer test cases #137 #149
Conversation
|
great that this has already been made, any specific reason why lists\sets are not supported? I have encountered them plenty of time in the wild. Also, sets could be rendered exactly the same, since the uniqueness of all items in a set should be checked in the pkl side. |
|
One way of doing this for the time being is as follows: I will add support for list/sets natively im just currently working on the ini std-lib. |
|
Thanks for the PR! Please also add The Java-side renderer is actually the less important renderer; those renderers are meant for Java users that want to take an already evaluated Java representation of a Pkl value. We only offer Java-side renderers for some formats. The more important renderer is the in-language renderer, via |
|
Ive been working on the in-language renderer over the past couple days. I have written the .pkl std lib. I have also added an I just need to write the @bioball i have a question regarding the |
|
The converters mechanism is a way for users to override the intrinsic behavior of the renderer. The standard library should be defining the intrinsic behavior, so, it's not appropriate to specify converters within For handling arrays: seems like there's no agreed upon format? Some implementations use commas, and others use square brackets, and others don't support them at all as far as I can tell. Maybe one way we can handle this is to add a renderer option for some commonly used styles. Where |
|
Thank you for your help. Test cases should be done today and i love the idea of adding the array style config to ini.pkl! |
|
All of what @bioball said and just to make the point explicit; the out-of-the-box-default behaviour of a renderer should always be restricted to target format standard representations. If there is no such standard, the default behaviour must be to error out. This preserves the principle of "least surprise." (This is the reason why, for example |
| class Person { | ||
| name: String | ||
| age: Int | ||
| address: Address | ||
| friend: Person? | ||
| } |
There was a problem hiding this comment.
This (in other tests) seems a hold-over from before we moved most examples to birds... By no means blocking, but better to use a more appropriate test case (non-blocking).
There was a problem hiding this comment.
Okay, Thank you. Ill have a look and update these tests with some more up to date ones
|
Thank you for confirming. Very helpful! |
There are still some test's that need to be added.
|
Pushed changes for code review purposes. I know i still need to add some more in-lang renderer tests 👍 |
| /* | ||
| These end functions group `VmTyped`, `VmMapping`, `VmDynamic`, `VmMap` at the end of the map and then writes them to builder | ||
| This is to comply with ini style standards. Please look at tests for example's | ||
| */ |
There was a problem hiding this comment.
For example if we look at ini renderer test 2
We want to render this Mapping:
mapping = new Mapping{
["one"] = "value1"
["two"] = new Person {name = "Pigeon" age = 19 address {
street = "fun Street"
}}
["three"] = "value3"
["four"] = 1.2
["five"] = new Mapping {
["six"] = 123
}
["seven"] = true
}
Without these changes to the end functions it would render out like this:
[mapping]
one = value1
[mapping.two]
name = Pigeon
age = 19
[mapping.two.address]
street = fun Street
three = value3
four = 1.2
[mapping.five]
six = 123
seven = true
Where with the changes to the end functions it renders out like this:
[mapping]
one = value1
three = value3
four = 1.2
seven = true
[mapping.two]
name = Pigeon
age = 19
[mapping.two.address]
street = fun Street
[mapping.five]
six = 123
Which matches the ini format rules
| // ignored for now | ||
| @Override | ||
| protected void endListing(VmListing value, boolean isEmpty) {} |
There was a problem hiding this comment.
For now? I think this may just be a good error message, no? I'm not very familiar with INI, but I don't think it has the equivalent. [Here, throughout]
There was a problem hiding this comment.
Okay 👍 i think your right with what you said, users can always overwrite the List converter if they want to add lists
|
Having an issue with the tests that overwrite the render functuons... So all the values get rendered out correctly but due to the way the ini renderer works using the end functuins to group up Maps, Dynamics etc, it renderes them still at the bottom of the file Im wondering if there is a flag somewhere that could be accessed by the end functions to tell it that the main render function for that data type has been overwritten? |
There was a problem hiding this comment.
General question: is there a specification that you are following here?
I'm having a hard time finding a specification for INI files. It seems like the closest is that different platforms provide their own spec for how they parse INI, and they're all slightly different.
Here's just some of the specs that I can find through my googling:
- https://cloanto.com/specs/ini/
- https://www.nongnu.org/chmspec/latest/INI.html
- https://moz-services-docs.readthedocs.io/en/latest/server-devguide/confspec.html
- https://spec.editorconfig.org/#file-format
The implementation here makes some choices that are incompatible with some of these specs. For example, non-string values are serialized, and how . in a section header means nesting, and using LF for line endings.
Is there a well known implementation that is being followed here?
Maybe the correct way to approach this is something like:
class IniRenderer {
flavor: "cloanto"|"somethingElse"
}
And using that as a way to determine how to render to INI?
Another alternative would be to break these options into their own properties.
class IniRenderer {
booleans: "truefalse"|"yesno"|"none" = "none"
lineEndings: "\r\n"|"\n" = "\r\n"
// etc
}
Either of these approaches have flaws, though. The first one is not flexible and wouldn't be able to capture some new INI format, and we'd have a hard time chasing down all of the variations and subtleties if taking the second approach.
|
|
||
| @Override | ||
| public void visitBoolean(Boolean value) { | ||
| writeKeyOrValue(value.toString()); |
There was a problem hiding this comment.
This is ambiguous; we can't tell apart string values that are "true" or "false" with booleans.
Same with the other primitives
As per @bioball suggestion Co-authored-by: Daniel Chao <[email protected]>
|
Have you had a chance to look at the issue i am having with the tests? |
|
It is annoying that ini files have such a loose format. With there being so many variations, to what extent can we then customize the renderer through converters?
Is this something that can be done through converters or should it be a renderer setting like bioball suggested above (or straight up not supprted)? My main concern is that exactly because the format is so loose, then there is a proliferation of dialects, but we should also be able to adapt this single renderer to any dialect (that doesn't take it too far) with ease. |
|
Indeed the format that i am basing this renderer off (For the time being) can be found here. |
@holzensp is there functionality there for the Renderer to tell if parts of it has been overwritten by pkl? |
@Madmegsox1: I'm not totally understanding the question. What do you mean by "main render function for that data type has been overwritten"? Can you share a snippet of the Pkl input, and your INI output, and your expected INI output? |
|
Sorry for the confusion by datatypes i mean the pkl converters. The reason this is happaning is because the ini renderer's end functions such as Just need to know if there is a flag somewhere could be passed to these endFunctions so they dont append the types? Here are the snip-its: Pkl Input Expected res1 = gnirts
res2 = false
res3 = 43
res4 = 2.33
res5 = 3 s
res6 = 4 mb
res8 = [\n 43,\n false,\n "gnirts"\n]
res9 = [\n "gnirts",\n false,\n 43,\n 5\n]
res10 = {\n "eno"\: false,\n "owt"\: 2.33,\n "eerht"\: "3 s"\n}
res11 = [\n "gnirts",\n false,\n 43,\n 5\n]
res12 = {\n "eman"\: "noegip",\n "ega"\: 31,\n "eerht"\: "3 s"\n}
res13 = {\n "name"\: "noegip",\n "age"\: 31,\n "other"\: "rehto"\n}
res14 = {\n "name"\: "noegip",\n "age"\: 41\n}
res15 = String
Actual output res1 = gnirts
res2 = false
res3 = 43
res4 = 2.33
res5 = 3 s
res6 = 4 mb
res8 = [\n 43,\n false,\n \"gnirts\"\n]
res9 = [\n \"gnirts\",\n false,\n 43,\n 5\n]
res10 = {\n \"eno\"\: false,\n \"owt\"\: 2.33,\n \"eerht\"\: \"3 s\"\n}
res11 = [\n \"gnirts\",\n false,\n 43,\n 5\n]
res12 = {\n \"eman\"\: \"noegip\",\n \"ega\"\: 31,\n \"eerht\"\: \"3 s\"\n}
res13 = {\n \"name\"\: \"noegip\",\n \"age\"\: 31,\n \"other\"\: \"rehto\"\n}
res14 = {\n \"name\"\: \"noegip\",\n \"age\"\: 41\n}
res15 = String
[res10]
eno = false
owt = 2.33
[res12]
eman = noegip
ega = 31
[res13]
name = noegip
age = 31
[res14]
name = noegip
age = StringThanks for the help @bioball |
|
Hi has anyone been able to take a look at this? |
| value.forceAndIterateMemberValues( | ||
| ((memberKey, member, memberValue) -> { | ||
| if ((memberValue instanceof VmTyped) | ||
| || (memberValue instanceof VmDynamic) | ||
| || (memberValue instanceof VmMapping) | ||
| || (memberValue instanceof VmMap)) { | ||
|
|
||
| if (memberKey instanceof Identifier) { | ||
| currPath.push(memberKey); | ||
| } else { | ||
| currPath.push(converter.convert(memberKey, List.of())); | ||
| } | ||
| visit(memberValue); | ||
| currPath.pop(); | ||
| } | ||
| return true; | ||
| })); |
There was a problem hiding this comment.
Should have caught this earlier; this is where your bug is. You already print the contents of an object by visiting each property before this method gets called (that provides the expected output). This is the bit that produces the unexpected extra output. The endDynamic / endTyped / endMap / etc methods are there for other kinds of admin, such as decreasing indent (in JSON, YAML, Pcf) and printing a closing } (in JSON, Pcf). You shouldn't revisit the members.
There was a problem hiding this comment.
Ah, Thank you. I understand where you are coming from, at the time i though it would be a good idea to use them for handeling complex Pkl files. See the issue comees when you have a pkl file like this for example:
mapping = new Mapping{
["one"] = "value1"
["two"] = new Person {name = "Pigeon" age = 19 address {
street = "fun Street"
}}
["three"] = "value3"
["four"] = 1.2
["five"] = new Mapping {
["six"] = 123
}
["seven"] = true
}
The expected output would have the correct sections such as mapping.one or mapping.two.address and then the values under said sections. For example:
[mapping]
one = value1
three = value3
four = 1.2
seven = true
[mapping.two]
name = Pigeon
age = 19
[mapping.two.address]
street = fun Street
[mapping.five]
six = 123
So the use of these "end functions" is to make sure that the values are correctly under each section. If i didnt use the end functions the rendered ini file would look like this:
[mapping]
one = value1
[mapping.two]
name = Pigeon
age = 19
[mapping.two.address]
street = fun Street
three = value3
four = 1.2
[mapping.five]
six = 123
seven = true
And as you can see some values such as seven and four are under the incorrect section.
Im not sure if you or anyone else could think of another way of overcoming this problem if so it would be great to get this solved!
There was a problem hiding this comment.
@holzensp have you had time to look at my issue?
There was a problem hiding this comment.
Again, sluggish; sorry. I get it now; you're having to compensate for .ini not having hierarchies, so things that would be nested, now come after the object is entirely rendered. You should still not revisit the object's members, though. I'd keep a stack of StringWriters in your renderer. Every startFoo you push a new one with the qualified name of the property already in it. Every endFoo you then pop it and write the top-writers contents.
Added ini renderer and ini renderer test cases.
Changes that still need to be added are as follows:
Ill create another pr later with these changes.
In
IniUtils, the list of charecters that need to be escaped within the .ini file are in accourdance with INI file Wiki.The
IniRendereris very similar to PropertiesRenderer due to .properties file having a similar style to an .ini file