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

Add GDS to JSON/ YAML/ TOML Conversion CLI #35

Merged
merged 12 commits into from
Dec 20, 2022
Merged

Conversation

dan-fritchman
Copy link
Owner

No description provided.

@dan-fritchman dan-fritchman requested a review from growly December 15, 2022 21:35
@growly
Copy link
Collaborator

growly commented Dec 15, 2022

Nice. Meta point: I wonder, is there a more memorable name for this tool? gds2ser is succinct and people will learn it, but consider:

gds2markup
gds2misc
gds2other
gds2inventedthismillenium
gds2modern

(Even something like gds2yaml even though it also does JSON would be more discoverable.)

Not a blocker.

Copy link
Collaborator

@growly growly left a comment

Choose a reason for hiding this comment

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

first pass

@@ -0,0 +1,2 @@

// FIXME: make this the shared part of Gds21 (De)Serialization CLIs
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this just a placeholder for a future file?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, that's right, and that ties into the binary-name.

I agree there should be (several) nicer names. E.g.

  • gds2json
  • gds2yaml
  • gds2toml
  • gds2{one of those things you wrote above}

And wanted to share the implementation of the former. But (a) wanted to use this today, and (b) couldn't immediately figure out the combination of sharing-as-library and creating-binaries.

match format {
"json" => Ok(SerializationFormat::Json),
"yaml" => Ok(SerializationFormat::Yaml),
"toml" => Err(format!("TOML is not yet supported, see https://github.com/dan-fritchman/Layout21/issues/33").into()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems more straightforward to just make this do what it will have to eventually do when TOML is fixed, and then fix #33

Copy link
Owner Author

Choose a reason for hiding this comment

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

Definitely agree it's more straightforward.

Problem is the error you get when just allowing TOML to fail natively is less than ideal, as described in #33 and in toml-rs/toml#396

Copy link
Owner Author

Choose a reason for hiding this comment

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

Probably worth adding -
it's not entirely clear that #33 is actually fixable.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think gds2markup (not "markdown") seems probably the best name the multi-format program currently called gds2ser?

Prefer anything else?

@dan-fritchman
Copy link
Owner Author

Adds a total of four short CLI programs:

  • gds2json
  • gds2yaml
  • gds2toml
  • gds2markup, which allows command-line option selection of the three formats

All four are (very) thin and use layout21converters::gds_serialization to do all their real work.

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