Tree refactoring and basic text writing #90

NightEule5 · 2022-01-10T08:52:01Z

No description provided.

Created the TomlTextWriter interface to handle low-level writing of Toml files. One implementation writes to a StringBuilder, the other writes to an Okio BufferedSink. KtomlConf has also been amended to include an "indentation" property.

This interface roughly corresponds to the kotlinx-serialization.json library's Composer interface in function, so the name was changed to reflect that. This also clears up potential confusion with TomlWriter and TomlFileWriter, which will have a different purpose.

Classes deriving from TomlValue now expose their content property in an internal constructor, and parsing was moved to a secondary constructor. Since member functions are inaccessible before the super constructor is called, private functions used for parsing were moved to companion objects.

NightEule5 · 2022-01-10T09:05:54Z

Hmm, not off to a good start with the workflows

I'll fix the formatting

Cleaned formatting and fixed style inconsistencies Diktat caught.

NightEule5 · 2022-01-10T12:14:08Z

Alright, fixed. Sorry about that

orchestr7 · 2022-01-10T12:23:48Z

@NightEule5 that's fucking awesome! Thank you so much
Give us several days to review

NightEule5 · 2022-01-10T13:56:45Z

Thanks! :)

Sure, no rush

Peanuuutz · 2022-01-10T16:53:56Z

Nice work. :D

A little suggestion, consider matching the type of content for each TomlValue.

BTW, I didn't find any actual class for tables. I may try to implement inline ones first. Shall I go with this thread or open a new PR? Also, should I defer inline ones and not inline ones? They behave quite differently. 🤔

orchestr7 · 2022-01-10T16:59:35Z

BTW, I didn't find any actual class for tables. I may try to implement inline ones first. Shall I go with this thread or open a new PR? Also, should I defer inline ones and not inline ones? They behave quite differently. 🤔

Hey! You can open a new PR, but I suppose that we will need to merge this huge PR first…
Is it ok for you?

Peanuuutz · 2022-01-10T17:03:20Z

Hey! You can open a new PR, but I suppose that we will need to merge this huge PR first...

I'm OK with that. I'll follow up when this is done. :D

NightEule5 · 2022-01-11T03:20:05Z

Nice work. :D

Thanks :)

A little suggestion, consider matching the type of content for each TomlValue.

Yep, I left it how it was for now. Constraining the type on an overridden var property gives an error, and I didn't try much yet to make that work

orchestr7 · 2022-01-11T15:08:05Z

Any chances that you will provide any tests that will cover this functionality? :)

orchestr7

As it does not affect decoding logic - we can merge it, but let's discuss comments, that I have written. It also will be great if you will add more comments to the code and give a small overview on your plan :)

orchestr7 · 2022-01-11T13:48:26Z

ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/KtomlConf.kt

-    val escapedQuotesInLiteralStringsAllowed: Boolean = true
-)
+    val escapedQuotesInLiteralStringsAllowed: Boolean = true,
+    val indentation: Indentation = FOUR_SPACES


Could you please add a trailing comma here, so we will avoid 2-lines changes in the future?

orchestr7 · 2022-01-11T13:58:01Z

ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/Toml.kt

@@ -24,6 +28,7 @@ public open class Toml(
 ) : StringFormat {
    // parser is created once after the creation of the class, to reduce the number of created parsers for each toml
    public val tomlParser: TomlParser = TomlParser(config)
+    public val tomlWriter: TomlWriter = TomlWriter(config)


Could you please add some comment to this writer for newbies?

Sure. Perhaps editing the comment above tomlParser to apply to both the parser and the writer would suffice? Or do you mean description of its function?

orchestr7 · 2022-01-11T14:22:39Z

ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/KtomlConf.kt

+    /**
+     * @property string
+     */
+    public enum class Indentation(public val string: String) {


may be call it value? string looks a little bit confusing, usually Java/Kotlin developers expect toString() or value

orchestr7 · 2022-01-11T14:55:00Z

ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/writers/TomlComposer.kt

+/**
+ * Abstracts the specifics of writing TOML files into "emit" operations.
+ */
+public interface TomlComposer {


Why do we need an interface if it is inherited only in one abstract class now? Any future plans?
I suggest to merge it with AbstractEmmiter if it is not used anywhere else

No specific plans for it, I just thought this way was best practice. I can merge it if that's preferable

Yes, it is a best practice when it has common sense. But here we interface it "just because" 😄
Better to merge, so the project won't be too huge to read for new developers

orchestr7 · 2022-01-11T14:59:06Z

ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/writers/AbstractTomlComposer.kt

+/**
+ * The based implementation of [TomlComposer].
+ */
+public abstract class AbstractTomlComposer(ktomlConf: KtomlConf) : TomlComposer {


Are you sure that Composer is a right name? I have seen Emitter in projects like 'snakeyaml'. What do you think, does it sound better?

Or you are trying to make it similar to kotlinx.Json?

Yes, that was the idea. Emitter is probably a more fitting and clear name though, so I'll change it

orchestr7 · 2022-01-11T15:17:50Z

ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/writers/TomlWriter.kt

+    public fun write(file: TomlFile, composer: TomlComposer): Unit =
+            file.children.forEach { composer.writeChild(it) }
+
+    private fun TomlComposer.writeChild(node: TomlNode) = when (node) {


You plan to insert Toml nodes to the Tree here? What is your plan on this code?
You plan to take an object -> create tree -> then emit string or file?

Yes, the plan here is to emit a tree that was created before to the Composer (Emitter), which then writes to a StringBuilder or BufferedSink depending on the implementation

orchestr7 · 2022-01-11T15:50:35Z

ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/writers/AbstractTomlComposer.kt

+    protected abstract fun emit(fragment: Char)
+
+    protected abstract fun emit(fragment1: String, fragment2: String)
+    protected abstract fun emit(


what is the user scenario for that bunch of emit methods? Do we really need all of them? Why?

The use case was for parts of the file that would need multiple emit calls at once, like strings (a quote, the value, another quote), comments, and indentation. In hindsight it'd be better to simplify this into two abstract emits, char and string

orchestr7 · 2022-01-11T16:02:42Z

ktoml-file/src/commonMain/kotlin/com/akuleshov7/ktoml/file/FileUtils.kt

+        val ktomlPath = tomlFile.toPath()
+        return getOsSpecificFileSystem().sink(ktomlPath).buffer()
+    } catch (e: FileNotFoundException) {
+        println("Not able to fine toml-file in the following path: $tomlFile")


fine -> find

orchestr7 · 2022-01-11T16:37:12Z

ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/writers/TomlWriter.kt

+
+    private fun TomlComposer.writeChild(node: TomlNode) = when (node) {
+        is TomlFile ->
+            error(


We should have proper exception instead of error here...

orchestr7 · 2022-01-11T17:05:49Z

ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/writers/TomlWriter.kt

+        val keys = key.keyParts
+
+        if (keys.isEmpty()) {
+            emitQuotedKey("")


keys can have a single quote also...

Oh, that's when the key is empty

NightEule5 · 2022-01-12T03:23:57Z

Any chances that you will provide any tests that will cover this functionality? :)

Yes, that's the plan :)

I can do that in a later PR since this one got too large

It also will be great if you will add more comments to the code and give a small overview on your plan :)

Sure, I'll add more documentation

My plan is to encode to the AST in the Encoder and CompositeEncoder implementation(s), then emit the AST via the appropriate Emitter. TomlWriter would traverse the tree, calling the necessary emit functions instead of writing the file or string directly.

At some point we could make the AST @Serializable like Kotlinx Json does, but that's beyond the scope here I think

Peanuuutz · 2022-01-12T04:05:33Z

My plan is to encode to the AST in the Encoder and CompositeEncoder implementation(s), then emit the AST via the appropriate Emitter. TomlWriter would traverse the tree, calling the necessary emit functions instead of writing the file or string directly.

For further development, comments in TOML file should be considered. I personally think annotation(such as @Comment) on properties is a good practice(then via descriptor.getElementAnnotation(index) to emit comments), but if models are encoded to AST, these annotations may be dicarded. Any solution to this? 🤔

OK I'm just trying to copy what I have done in my implementation

NightEule5 · 2022-01-13T05:35:24Z

For further development, comments in TOML file should be considered. I personally think annotation(such as @Comment) on properties is a good practice(then via descriptor.getElementAnnotation(index) to emit comments), but if models are encoded to AST, these annotations may be dicarded. Any solution to this?

Yes, I think this is a good idea. We'll have to add comments to the AST later

Other annotations would be good too, like @Multiline and @Literal for strings, @Integer(representation=...) for longs, and @Inline for classes and maps

orchestr7 · 2022-01-14T12:20:23Z

@NightEule5

Other annotations would be good too, like @Multiline and @Literal for strings, @Integer(representation=...) for longs, and @Inline for classes and maps

could you please describe this Annotation logic here: #79

And let's merge this PR when you are ready. But please use Squash and merge.

NightEule5 added 5 commits December 25, 2021 23:15

Create TomlTextWriter and its implementations

99c90dd

Created the TomlTextWriter interface to handle low-level writing of Toml files. One implementation writes to a StringBuilder, the other writes to an Okio BufferedSink. KtomlConf has also been amended to include an "indentation" property.

Create TomlWriter and TomlFileWriter

a3d3d1e

Move AST classes out of the "parsers" package

8962396

NightEule5 linked an issue Jan 10, 2022 that may be closed by this pull request

Support encoding of toml format #11

Closed

NightEule5 added 3 commits January 10, 2022 04:04

Prepare for pull request

c85313d

Cleaned formatting and fixed style inconsistencies Diktat caught.

Fix unresolved reference missed by diktatFix

d795753

Remove reference to uncommitted code

63300a5

orchestr7 reviewed Jan 11, 2022

View reviewed changes

orchestr7 mentioned this pull request Jan 11, 2022

Tweak config and exception #91

Merged

NightEule5 added 3 commits January 11, 2022 20:27

Implement PR suggestions and corrections

be49283

Add TODO to unimplemented branches

8202750

Fix import ordering

b685378

orchestr7 mentioned this pull request Jan 13, 2022

Support setting execCmd from frontend saveourtool/save-cloud#518

Merged

Document TomlEmitter

0664aa0

orchestr7 approved these changes Jan 14, 2022

View reviewed changes

NightEule5 merged commit d953779 into orchestr7:main Jan 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tree refactoring and basic text writing #90

Tree refactoring and basic text writing #90

NightEule5 commented Jan 10, 2022

NightEule5 commented Jan 10, 2022 •

edited

Loading

NightEule5 commented Jan 10, 2022

orchestr7 commented Jan 10, 2022 •

edited

Loading

NightEule5 commented Jan 10, 2022

Peanuuutz commented Jan 10, 2022 •

edited

Loading

orchestr7 commented Jan 10, 2022

Peanuuutz commented Jan 10, 2022

NightEule5 commented Jan 11, 2022

orchestr7 commented Jan 11, 2022

orchestr7 left a comment

orchestr7 Jan 11, 2022

orchestr7 Jan 11, 2022

NightEule5 Jan 12, 2022 •

edited

Loading

orchestr7 Jan 11, 2022

orchestr7 Jan 11, 2022

NightEule5 Jan 12, 2022

orchestr7 Jan 12, 2022

orchestr7 Jan 11, 2022

NightEule5 Jan 12, 2022

orchestr7 Jan 11, 2022

NightEule5 Jan 12, 2022

orchestr7 Jan 11, 2022

NightEule5 Jan 12, 2022

orchestr7 Jan 11, 2022

orchestr7 Jan 11, 2022

orchestr7 Jan 11, 2022

NightEule5 Jan 12, 2022

NightEule5 commented Jan 12, 2022

Peanuuutz commented Jan 12, 2022 •

edited

Loading

NightEule5 commented Jan 13, 2022 •

edited

Loading

orchestr7 commented Jan 14, 2022 •

edited

Loading

Tree refactoring and basic text writing #90

Tree refactoring and basic text writing #90

Conversation

NightEule5 commented Jan 10, 2022

NightEule5 commented Jan 10, 2022 • edited Loading

NightEule5 commented Jan 10, 2022

orchestr7 commented Jan 10, 2022 • edited Loading

NightEule5 commented Jan 10, 2022

Peanuuutz commented Jan 10, 2022 • edited Loading

orchestr7 commented Jan 10, 2022

Peanuuutz commented Jan 10, 2022

NightEule5 commented Jan 11, 2022

orchestr7 commented Jan 11, 2022

orchestr7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NightEule5 Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NightEule5 commented Jan 12, 2022

Peanuuutz commented Jan 12, 2022 • edited Loading

NightEule5 commented Jan 13, 2022 • edited Loading

orchestr7 commented Jan 14, 2022 • edited Loading

NightEule5 commented Jan 10, 2022 •

edited

Loading

orchestr7 commented Jan 10, 2022 •

edited

Loading

Peanuuutz commented Jan 10, 2022 •

edited

Loading

NightEule5 Jan 12, 2022 •

edited

Loading

Peanuuutz commented Jan 12, 2022 •

edited

Loading

NightEule5 commented Jan 13, 2022 •

edited

Loading

orchestr7 commented Jan 14, 2022 •

edited

Loading