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

Support encoding of toml format #11

Closed
orchestr7 opened this issue Apr 11, 2021 · 10 comments · Fixed by #90 or #153
Closed

Support encoding of toml format #11

orchestr7 opened this issue Apr 11, 2021 · 10 comments · Fixed by #90 or #153
Assignees
Labels
enhancement New feature or request huge task

Comments

@orchestr7
Copy link
Owner

orchestr7 commented Apr 11, 2021

Decoder has the first priority as the Toml usually is used for configuration files, but encoders should also be supported

@orchestr7 orchestr7 self-assigned this Apr 11, 2021
@orchestr7 orchestr7 added enhancement New feature or request huge task labels Jul 11, 2021
@NightEule5
Copy link
Collaborator

NightEule5 commented Dec 31, 2021

I'm working on a rough first implementation of this functionality in this branch. I'm wondering if you have any thoughts on how it should look. Any input is welcome. :)

Mainly, should encoding be done to an AST then written to a file or string, or directly? The KotlinX Json format has encoders for both: StreamingJsonEncoder for writing directly to a string and TreeJsonEncoder for encoding to a tree. From the latter, the Json tree itself is serializable

For the tree-based approach, the Toml AST in parsers.node was obviously written with only parsing in mind. It'd have to be either adapted for both or a separate tree would need to be made (I like the former better, but it'd take some effort).

In the meantime, I'll go forward with it. I can make changes later if needed

@orchestr7
Copy link
Owner Author

@NightEule5 hello! And thank you so much for this! This is incredible, you can open PR as soon as you will be ready.

Speaking about the AST. We can treat it as an Intermediate Representation. Even right now in Ktoml we have some logic that can help user to read only table names/modify AST on the fly and so on. These things could be useful for encoding also.

So I suppose we need to keep AST (or any other IR) in place. May be we can modify the existing AST somehow?

@orchestr7
Copy link
Owner Author

orchestr7 commented Dec 31, 2021

@NightEule5 will be great if you would take ownership over the serialization part if it is ok for you.

@NightEule5
Copy link
Collaborator

Thanks for the response!

So I suppose we need to keep AST (or any other IR) in place. May be we can modify the existing AST somehow?

Yes, I think so. Currently, it looks like much of the parsing happens in the AST classes themselves, right? Maybe we could add a second constructor to the AST classes. If no value is passed for the content property, it could encode the child nodes when invoked

@NightEule5
Copy link
Collaborator

@NightEule5 will be great if you would take ownership over the serialization part of it is ok for you.

Sure, I'd appreciate that

@orchestr7
Copy link
Owner Author

@NightEule5 I also suggest you to have a look at what’s Charles is doing in https://github.com/charleskorn/kaml

I love his implementation, may be we can follow his approach after modifying the tree a little bit.

The only thing I am asking about - to split this task into smaller parts, to make PRs small…

@orchestr7
Copy link
Owner Author

orchestr7 commented Dec 31, 2021

Currently, it looks like much of the parsing happens in the AST classes themselves, right?

Yes, I tried to make it OOP-like. Every Node is an instance where the logic and validation is done. But there are useful interfaces, that can help us to build a tree from class during the serialization process.

@NightEule5
Copy link
Collaborator

Sure, got it

@NightEule5
Copy link
Collaborator

Ok, I've refactored the node tree a bit and created low-level file/string writing classes, and tests still pass. Should I create a pull request to the main branch or should we keep it in a separate branch until it's complete?

@orchestr7
Copy link
Owner Author

orchestr7 commented Jan 10, 2022

Ok, I've refactored the node tree a bit and created low-level file/string writing classes, and tests still pass. Should I create a pull request to the main branch or should we keep it in a separate branch until it's complete?

@NightEule5 could you please create a PR to this repo to main? may be I can help with some review..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request huge task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants