Skip to content
This repository has been archived by the owner on Sep 24, 2022. It is now read-only.

Add the preserve_order feature (rebased version) #278

Merged
merged 3 commits into from
Jan 8, 2019

Conversation

Kerollmops
Copy link
Contributor

@Kerollmops Kerollmops commented Dec 28, 2018

Here is the #242 PR from @Keats rebased on master.

Therefore it closes #242.

@alexcrichton
Copy link
Collaborator

Thanks!

@ehuss you mentioned that you're working on a toml parser preserving style, I'm curious if it'd be at all related to something like this? Were you thinking that'd be a totally standalone project?

(mostly looking to minimize major version bumps)

@ehuss
Copy link
Collaborator

ehuss commented Jan 8, 2019

🤔 I'm not sure, I've been meaning to talk to you about that. It could go either way. Right now I'm developing it in tree. If it is a separate crate, toml-rs will need to expose a few low-level methods and data structures which doesn't sound too appealing. However, it's a fairly large amount of code and I don't want to just dump it on the project. There's at least a few weeks of work left to do, so maybe once it's more together we could take a look and discuss the options?

As for the relation to this, I think code-wise it is independent. I'm not really touching the serde or Value parts.

There's maybe a bigger question of sharing code with serde_json. For example, to really support #266 it might be nice to use the Number type from serde_json. There's a lot of good stuff in serde_json, and just copying it seems a little weird.

@alexcrichton
Copy link
Collaborator

Ah ok, do you know if the changes you'd need, if it goes in this repository, will require breaking changes? I'm fine evaluating the best location for the work once it's done!

I'm somewhat wary about Number and such as integers in toml are apparently defined as signed 64-bit integers, so while we should handle 128-bit integers I don't think we can handle values outside the range of a 64-bit signed integer.

@ehuss
Copy link
Collaborator

ehuss commented Jan 8, 2019

The only changes I see so far is adding some new pub items. In particular:

  • Deserializer::line — this is the main parsing function that will return everything including whitespace and comments.
  • Line enum (returned by line)
  • The "raw" key and value types from Deserializer that will contain whitespace/comments.
  • Deserializer::value and Deserializer::dotted_key for parsing just a key or value.

Right now I have them pub(crate) which would unfortunately require a minimum bump to 1.18. Otherwise I don't see any other fundamental changes.

Good point on the integers.

@alexcrichton
Copy link
Collaborator

Ok! In that case I'm gonna go ahead and merge this as it sounds like we probably won't need another major version bump

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants