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

[Addition] Added IniRenderer and IniRenderer test cases #137 #149

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Madmegsox1
Copy link

Added ini renderer and ini renderer test cases.
Changes that still need to be added are as follows:

  • a new in-language renderer
  • a new standard library module, as mentioned in this issue.

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 IniRenderer is very similar to PropertiesRenderer due to .properties file having a similar style to an .ini file

@ghyatzo
Copy link

ghyatzo commented Feb 15, 2024

great that this has already been made, any specific reason why lists\sets are not supported?
something like this:

[section]
mylist=[a, list, of, things]
; or even
myotherlist=another,list,of,things

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.

@Madmegsox1
Copy link
Author

Madmegsox1 commented Feb 15, 2024

One way of doing this for the time being is as follows:

output {
   renderer = new IniRenderer {
     converters {
       // render lists as comma-separated values
       [List] = (it) -> it.join(",")
    }
  }
}

I will add support for list/sets natively im just currently working on the ini std-lib.

@bioball
Copy link
Contributor

bioball commented Feb 15, 2024

Thanks for the PR! Please also add ini.pkl to this PR too.

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 ini.pkl, and I don't think we want the Java-side in without the in-language renderer also being there.

@Madmegsox1
Copy link
Author

Madmegsox1 commented Feb 15, 2024

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 IniModule class and the ini/RenderNodes class.

I just need to write the LanguageSnippetTest.

@bioball i have a question regarding the ini.pkl stb-lib. As mentioned in the comment above regarding using converters { [List] = (it) -> it.join(",") } could this be done to convert lists inside ini.pkl instead of doing it inside of ini/RenderNodes. There isnt any particular reason i just think it would be cool to do it inside of the stb-lib.

@bioball
Copy link
Contributor

bioball commented Feb 15, 2024

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 stdlib/ini.pkl. Also: for the standard library, we want to define everything in Java, because that will be faster than something implemented in Pkl.

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.

class Renderer {
  arrayStyle: ("comma"|"bracket")?
}

Where null means: throw if any List or Listing are found.

@Madmegsox1
Copy link
Author

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!

@holzensp
Copy link
Contributor

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 JsonRenderer doesn't know how to deal with Durations, even though there are perfectly reasonable default choices.)

Comment on lines +1 to +6
class Person {
name: String
age: Int
address: Address
friend: Person?
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

Okay, Thank you. Ill have a look and update these tests with some more up to date ones

@Madmegsox1
Copy link
Author

Thank you for confirming. Very helpful!

There are still some test's that need to be added.
@Madmegsox1
Copy link
Author

Pushed changes for code review purposes. I know i still need to add some more in-lang renderer tests 👍

Comment on lines +267 to +270
/*
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
*/
Copy link
Author

@Madmegsox1 Madmegsox1 Feb 18, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@holzensp holzensp left a comment

Choose a reason for hiding this comment

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

Coming along nicely.

Comment on lines +313 to +315
// ignored for now
@Override
protected void endListing(VmListing value, boolean isEmpty) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

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]

Copy link
Author

Choose a reason for hiding this comment

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

Okay 👍 i think your right with what you said, users can always overwrite the List converter if they want to add lists

@Madmegsox1
Copy link
Author

Madmegsox1 commented Feb 20, 2024

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

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 = String

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?

Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

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

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:

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());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ambiguous; we can't tell apart string values that are "true" or "false" with booleans.

Same with the other primitives

pkl-core/src/main/java/org/pkl/core/IniRenderer.java Outdated Show resolved Hide resolved
@Madmegsox1
Copy link
Author

Have you had a chance to look at the issue i am having with the tests?

@ghyatzo
Copy link

ghyatzo commented Feb 28, 2024

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?
For example, some more variations that I have encountered in the wild:

  • using : instead of = for the key-value separator
  • no nesting, but instead everything is flattened into a single section

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.
Luckily an ini file doesn't have that many moving parts to generalize.

@Madmegsox1
Copy link
Author

Madmegsox1 commented Feb 28, 2024

Indeed the format that i am basing this renderer off (For the time being) can be found here.
Currently im trying to fix some of the tests but need some of the maintainers help on a issue i am having the tests.
Bioballs suggestion by letting the usere choose how to rendere the file will be implemented once i have fixed these tests

@Madmegsox1
Copy link
Author

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

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 = String

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?

@holzensp is there functionality there for the Renderer to tell if parts of it has been overwritten by pkl?

@bioball
Copy link
Contributor

bioball commented Feb 28, 2024

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?
@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?

@Madmegsox1
Copy link
Author

Madmegsox1 commented Feb 29, 2024

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 endMapping or endDynamic group VmTyped, VmMapping, VmDynamic, VmMap at the end of the type and then writes them to the builder and im guessing the reason they are still doing this when the converters are overwritten from pkl is because these end functions are still called which is why these types in the tests appear at the bottom of the output.

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

import "pkl:ini"

class Person {
  name: String
  age: Int?
}

res1 = "string"
res2 = true
res3 = 42
res4 = 1.23
res5 = 3.s
res6 = 4.mb
res8 = List("string", true, 42)
res9 = Set("string", true, 42)
res10 = Map("one", true, "two", 1.23)
res11 = new Listing { "string"; true; 42 }
res12 = new Mapping { ["name"] = "pigeon"; ["age"] = 30 }
res13 = new Dynamic { name = "pigeon"; age = 30 }
res14 = new Person { name = "pigeon" }
res15 = null

output {
  renderer = new ini.Renderer {
    omitNullProperties = false
    converters = (jsonValueRenderer.converters) {
      [List] = (it) -> jsonValueRenderer.renderValue(it.reverse())
      [Set] = (it) -> jsonValueRenderer.renderValue(it + List(4))
      [Map] = (it) -> jsonValueRenderer.renderValue(it + Map("three", 3.s))
      [Listing] = (it) -> jsonValueRenderer.renderValue((it) { 4 })
      [Mapping] = (it) -> jsonValueRenderer.renderValue((it) { ["three"] = 3.s })
      [Dynamic] = (it) -> jsonValueRenderer.renderValue((it) { other = "other" })
      [Person] = (it) -> jsonValueRenderer.renderValue((it) { age = 40 } /* fill in missing property */)
      [Pair] = (it) -> jsonValueRenderer.renderValue(List(it.second, it.first))
      [IntSeq] = (it) -> jsonValueRenderer.renderValue(List(it.start, it.end, it.step))
    }
  }
  local jsonValueRenderer = new JsonRenderer {
    converters {
      [String] = (it) -> it.reverse()
      [Boolean] = (it) -> !it
      [Int] = (it) -> it + 1
      [Float] = (it) -> it + 1.1
      [Duration] = (it) -> "\(it.value) \(it.unit)"
      [DataSize] = (it) -> "\(it.value) \(it.unit)"
      [Null] = (it) -> "String"
    }
  }
}

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 = String

Thanks for the help @bioball

@Madmegsox1
Copy link
Author

Hi has anyone been able to take a look at this?

Copy link
Contributor

@holzensp holzensp left a comment

Choose a reason for hiding this comment

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

Very sorry for letting this sit; travel and other distractions. I believe I've found your issue, though.

Comment on lines +294 to +310
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;
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@Madmegsox1 Madmegsox1 Apr 5, 2024

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

@holzensp have you had time to look at my issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much! Ill have a look soon

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.

4 participants