search

sifive / wit

Workspace Integration Tool
Apache License 2.0
23 stars 13 forks source link

Consolidate json format parsing #244

Closed mmjconolly closed 4 years ago

mmjconolly commented 4 years ago

Unlike https://github.com/sifive/wit/pull/242 this does not change the formats.

These 3 file types now use the same parse/unparse/write/read code

Most of the change is the addition of repo_entries.py and the rest of the changes are to use the new code from that file.

While formats themselves remain unchanged, this refactoring should make it easier for us to support new formats while maintaining compatibility.

As a future direction, I'm thinking that by checking the path name of a file we can know to write it as a, for example, wit-lock.json-style. Where as if it's called new-format.toml perhaps we could read out the version number from the current on-disk file to see what version we should write out.

mmjconolly commented 4 years ago

It especially feels that most of the functionality of Manifest, Lock, and maybe even Package? is redundant with RepoEntry/RepoEntries. I think one difference between some of the data types is that some just represent the JSON files (the "spec"), and other represent internal state that changes during the course of the Wit operation?

The dependency/package -> repoentry and workspace/manifest -> [repoentry] is something I've been thinking about for awhile and feel like it could be cleaned up. My main intent with this PR was to change as little as possible and follow up with more refactors over time

richardxia commented 4 years ago

The dependency/package -> repoentry and workspace/manifest -> [repoentry] is something I've been thinking about for awhile and feel like it could be cleaned up. My main intent with this PR was to change as little as possible and follow up with more refactors over time

Yeah, I understand, and I didn't mean to imply that we should change it now, but I thought that even a tiny bit of documentation could help provide clarity on where we're going. I guess don't worry about it, especially if there are some other PRs coming up that are going to change them anyway.

richardxia commented 4 years ago

Whoops, I had a stale browser tab open where it looked like I had not submitted an updated review.