Support format json5 #142
Support format json5 #142
Conversation
There was a problem hiding this comment.
There seem to be some more unnecessary scopes I did not comment.
Also, the diff includes code format changes which have nothing to do with this PR, which should be filed in another PR (or which are already filed in #155).
|
Are you still interested in this? If yes, I created a maintenance fork (read here). |
There was a problem hiding this comment.
So, I did a review of all changes in this PR.
Overall I have to say that this goes into the right direction, I guess. Still, I have quite a few remarks.
In general: Could you please rebase to master? This way the merge commits would vanish, which is particularly important to me, as it cleans the history quite a bit. This PR is based on a really old commit and I guess some of my remarks here only exist because of the merge, which makes this diff here appear to have changes which are made on master and not in this PR. I hope. If they are indeed from this PR, please make sure to file changes that do not conceptionally belong to the json5 feature in seperate PRs. But I'd bet, as said, that most of my remarks render invalid after your rebase! 👍
Also, please make clippy happy! 😆
|
Remove all nonrelative files, enjoy 😄 |
|
Nice. Now please make it run successfully through CI, then I'll do another review. |
All green now, :) |
| @@ -95,6 +95,18 @@ impl ConfigError { | |||
| } | |||
| } | |||
|
|
|||
| // Have a proper error fire if the root of a file is ever not a Table | |||
| // TODO: for now only json5 checked, need to finish others | |||
There was a problem hiding this comment.
Is this something that needs to be done but slipped? 😆
There was a problem hiding this comment.
Well, this TODO is from something like this: https://github.com/mehcode/config-rs/blob/266f504d9f23e192c03ef486f58a678847249b60/src/file/format/hjson.rs#L11
| } | ||
|
|
||
| Value::new(uri, ValueKind::Table(m)) | ||
| } |
There was a problem hiding this comment.
No, they are not. In the current version, you're iterating over a (ref to key, ref to value) tuple and then clone() the key when inserting into the hashmap.
In my version, you would iterate over the values and move them, no cloning involved.
|
Nice. But no need to merge master, please remove that, it makes review really hard. |
|
@matthiasbeyer I guess what you meant is rebase, not remove. I have to choose merge or rebase, otherwise it won't pass CI testing. I pushed the rebase version. enjoy. |
|
Thanks for your contribution! json5 support was merged in #206 |
😄