Skip to content

Use JSON for internal platform config, to avoid TOML issues - #220

Merged
dherman merged 4 commits into
volta-cli:masterfrom
mikrostew:toml-ordering
Dec 18, 2018
Merged

Use JSON for internal platform config, to avoid TOML issues#220
dherman merged 4 commits into
volta-cli:masterfrom
mikrostew:toml-ordering

Conversation

@mikrostew

Copy link
Copy Markdown
Contributor

Closes #219.

@mikrostew

Copy link
Copy Markdown
Contributor Author

Now this is what happens when I run the commands from that bug:

$ notion install node 11

$ cat ~/.notion/tools/user/platform.toml
[node]
runtime = '11.4.0'
npm = '6.4.1'

$ notion install yarn 1.9.2

$ cat ~/.notion/tools/user/platform.toml
yarn = '1.9.2'

[node]
runtime = '11.4.0'
npm = '6.4.1'

@mikrostew
mikrostew requested a review from dherman December 15, 2018 07:55

@dherman dherman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple suggestions.

Another thought that occurs to me is that TOML is optimized for human-readability, which is where some of these complexities come from. Maybe for internal files like platform.toml we'd be better off with JSON. IOW, the policy would be: TOML for human-maintained files (like the hooks configuration), and JSON for files representing internal state. Just a thought; we could also decide later.

Comment thread crates/notion-core/src/toolchain/serial.rs Outdated
Comment thread crates/notion-core/src/toolchain/mod.rs Outdated
@mikrostew mikrostew changed the title Fix ordering to satisfy TOML serialization Use JSON for internal platform config, to avoid TOML issues Dec 17, 2018

@dherman dherman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@dherman
dherman merged commit 078d1ef into volta-cli:master Dec 18, 2018
@mikrostew
mikrostew deleted the toml-ordering branch December 18, 2018 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants