Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
  • Nov 28 12:44
    epage closed #375
  • Nov 28 12:44
    epage commented #375
  • Nov 28 01:25
    tu6ge opened #375
  • Nov 24 17:45
    PopFlamingo edited #373
  • Nov 24 17:30
    wcarmon opened #374
  • Nov 24 08:40
    PopFlamingo opened #373
  • Nov 23 17:16
    DavidKorczynski opened #372
  • Nov 21 23:51
    epage commented #371
  • Nov 21 23:49
    epage commented #371
  • Nov 21 23:34
    Kixunil commented #371
  • Nov 21 18:46
    epage commented #371
  • Nov 21 18:42
    ordian commented #371
  • Nov 21 18:30
    Kixunil opened #371
  • Nov 20 02:14
    z-Wind closed #370
  • Nov 20 01:45
    epage labeled #369
  • Nov 20 01:45
    epage labeled #369
  • Nov 20 01:45
    epage commented #369
  • Nov 19 23:50
    epage commented #370
  • Nov 19 10:02
    z-Wind opened #370
  • Nov 18 23:54
    ssmendon opened #369
Pascal Hertleif
@killercup
@ordian i'll just dump some random thought after my first 5min of reading toml_edit code here :)
first off: this is an awesome code base!
could use some comment on the abstract software design (how the pieces of code relate/interact with each other) but with Go To Definition it's actually quite easy to navigate
(and you said the API is not final, so it might not be a good idea to invest in docs right now anyway)
one thing that surprised me was the value::ValueType enum
Pascal Hertleif
@killercup
i think this is only used in value::Array::push_value right now
but it could be used to implement nice indexing, serde_json style
(also cf alexcrichton/toml-rs#193)
on a most abstract level, i think the API should try to be close to that of serde_json, because a lot of thought has been put into making that nice
and then, on top of that, let's add custom methods like insert_at, and sort_entries_by
do you have any plans to make this is general toml crate? i.e., add serde support?
Pascal Hertleif
@killercup
i tried to run the fuzzer btw and it immediately crashes with a heap-use-after-free on my mac. i'll try to see if it's a local thing
(nice to see you downloading the run-fuzzer.sh script I wrote for rust-fuzz/targets :D)
Andronik Ordian
@ordian
hey, @killercup! Thanks for sharing your thoughts!
About indexing: yeah serje_json indexing was an inspiration for that struct.
About fuzz: I wasn't able to run fuzzer on my local machine due to some asan error, but I've seen some false positives when running sanitizers.
The current problem with making api as nice as serde_json has is that serde_json and toml-rs have a single enum - Value, while in toml_edit we have Value, ArrayOrTables, Table - and they are different in the way you can own them. E.g. you can only have a reference to Table, while Value permits full ownership semantics.
Pascal Hertleif
@killercup
the fuzz thing might just be a data structure usage asan doesnt understand
Andronik Ordian
@ordian
Shadow memory range interleaves with an existing memory mapping. ASan cannot proceed correctly.
Pascal Hertleif
@killercup
is it possible for us to use a single Value enum?
if this is not meant to be used to parse 10GB files, it's fine use just add a bunch of Rc/Arc
Andronik Ordian
@ordian

is it possible for us to use a single Value enum?

Well, we have TableChild and TableChildMut, this basically covers all types (except for the document).

if this is not meant to be used to parse 10GB files, it's fine use just add a bunch of Rc/Arc

The problem with Rc is not only performance consideration, but also that it's painful to use without non-lexical lifetimes. Actually early version used Rc as pointer to the document in table and array of tables, but I switched to raw pointers later (it's was before initial public commit).

Andronik Ordian
@ordian
I meant Rc<RefCell<T>>, not just Rc<T>.
Pascal Hertleif
@killercup
ah, true. well, the nll rfc is up, so it's only about a year away ;)
André Zanellato
@AZanellato
Hello :)
@ordian I saw you mentioning that the cargo-add command was blocked by the two issues on toml_edit. Would love to help with that
I don't have that much Rust experience, but I would try me best :smile:
Andronik Ordian
@ordian
@AZanellato hi there, glad to hear that :) I'll add more context to #57 soon. You can post any questions here and I'll try to respond in a timely fashion.
André Zanellato
@AZanellato
Nice :smile:
I'll try to check something later tonight after my working hours are done
ironyman
@ironyman
I would like to help too :D
Andronik Ordian
@ordian
@ironyman hello! Would you be interested in adding toml 0.5 support? ordian/toml_edit#58
This would require changing a parser (https://github.com/ordian/toml_edit/tree/master/src/parser) as well as e.g. adding new Value type "dotted keys".
André Zanellato
@AZanellato
Hi @ordian ! So, I'm checking the codebase and from what I am gathering I would need to change the document parser here: https://github.com/ordian/toml_edit/blob/master/src/parser/document.rs Which uses the macros defined here:
https://github.com/ordian/toml_edit/blob/master/src/parser/macros.rs
Is that it? :smile:
ironyman
@ironyman
sure
Andronik Ordian
@ordian
@AZanellato I guess that would require changing all the parsers, including the macros. The implementation of Document::from_str would first convert &str to Token stream and then use document parser on top of tokens. This probably means changing error type to include Token error.
André Zanellato
@AZanellato
Yeah, that makes sense :smile:
I have been reading the parser that you posted on #57 and taking a look at the codebase first but will let you know as soon as I made some progress :)
André Zanellato
@AZanellato
@ordian do you have any reading material on parsers I could use as well? :)
Andronik Ordian
@ordian
@AZanellato hey, for beginners I would recommend http://www.craftinginterpreters.com/contents.html chapters 4-6. Also searching for "parser combinator" would help, since it is related to the crate (combine) used for parsing. Hope that helps :)
André Zanellato
@AZanellato
Nice! Thank you very much. I was checking a lot of stuff about combine and that has been useful in understanding the code and how I can extend it
I was really hoping I could contribute faster, but I need to do some studying first :sweat_smile:
Andronik Ordian
@ordian
@AZanellato no worries, take your time and don't hesitate to ask any questions no matter how "dumb" they seem ;)
André Zanellato
@AZanellato
Thanks! :smile:
André Zanellato
@AZanellato
Just to keep you guys posted, I've been studying parser and am still interested in doing this
Been quite busy this last week with my masters, but will come back ASAP :)
ironyman
@ironyman
Hey @ordian , I have a WIP. I added a t! test. My test is failing because serde is serializing a more verbose string for the toml document than the json one but they seem to be equivalent. Do you have any advice? https://github.com/ironyman/toml_edit/blob/master/tests/test_valid.rs#L382
thread 'test_dotted_key' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
 Object(
     {
<        "name": String(
<            "Orange",
>        "name": Object(
>            {
>                "type": String(
>                    "string",
>                ),
>                "value": String(
>                    "Orange",
>                ),
>            },