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

Generate JSON instead of YAML #304

Merged
merged 1 commit into from
Oct 31, 2024
Merged

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Oct 27, 2024

Previously this generate a instr_dict.yaml file, however there is no need to use YAML here. The only advantages of YAML over JSON are that it is (debatably) easier for humans to write, making it suitable as human-edited input files. This is a machine-generated output file so JSON is better.

The main advantages are:

  1. It removes the dependency on PyYAML, which is the only external dependency of this project. Python external dependencies are quite a pain - PyYAML is a particularly troublesome one - and having no dependencies is very convenient (no need for venv etc.).
  2. It means consumers of the file don't need to depend on PyYAML.

Note this could have been done in an 100% backwards compatible way by keeping the file name unchanged (since JSON is valid YAML), however I figured there probably aren't that many users of this file, and since the only thing they need to change is the filename it's probably better to minimise confusion by renaming it. It also makes it clear that it is guaranteed to be JSON.

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 44.48%. Comparing base (25c09e6) to head (0a4d5e7).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
parse.py 33.33% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

❗ There is a different number of reports uploaded between BASE (25c09e6) and HEAD (0a4d5e7). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (25c09e6) HEAD (0a4d5e7)
2 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #304       +/-   ##
===========================================
- Coverage   95.87%   44.48%   -51.39%     
===========================================
  Files          10       10               
  Lines         752      744        -8     
===========================================
- Hits          721      331      -390     
- Misses         31      413      +382     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@IIITM-Jay
Copy link
Member

Just keeping my thoughts here @aswaterman regarding the above mentioned points in the PR description

This is a machine-generated output file so JSON is better.

@Timmmm Even though this is machine-generated, YAML’s indentation-based structure makes large, deeply nested data easier to read at a glance than JSON’s curly-brace heavy syntax. It is easier to review and debug faster comparable to JSON structure.

For an example, YAML

instructions:
  - name: ADD
    opcode: 0x01
    description: "Adds two numbers"

and JSON,

{
  "instructions": [
    {
      "name": "ADD",
      "opcode": "0x01",
      "description": "Adds two numbers"
    }
  ]
}
  • Although the current output is machine-generated, requirements might change in the future. YAML offers more flexibility to grow organically with minimal changes.
    • For example, if configuration overrides, custom templates, or metadata annotations become necessary in the future, YAML will handle them more elegantly.
    • Choosing YAML ensures the project is prepared for evolving needs without requiring a switch back from JSON later.

It removes the dependency on PyYAML, which is the only external dependency of this project. Python external dependencies are quite a pain

  • While the argument against PyYAML is reasonable (avoiding external dependencies), YAML parsing is trivial in modern Python. Additionally, Python’s standard library has added support for TOML (another human-friendly format), showing a trend toward readable config formats. In contrast, JSON is less suited for these roles, potentially making YAML a better long-term choice.

Note this could have been done in an 100% backwards compatible way by keeping the file name unchanged (since JSON is valid YAML)

Absolutely correct, YAML files are a superset of JSON. If a consumer prefers JSON, they can parse the YAML as JSON directly, without needing PyYAML. Here I could say that: Keeping YAML does not limit interoperability—it just offers more flexibility.

parse.py Outdated Show resolved Hide resolved
@Timmmm
Copy link
Contributor Author

Timmmm commented Oct 28, 2024

It is easier to review and debug faster comparable to JSON structure.

Well, I could debate the "improved" readability of YAML. Would you have guessed from the YAML that 0x01 is a string? I would have to look that up, whereas it's very clear in the JSON.

But in any case I don't think you can argue that the difference in readability is significant, whereas the benefit of not relying on PyYAML and not requiring consumers to deal with YAML are undeniably large.

@Timmmm Timmmm force-pushed the json_output branch 2 times, most recently from b56d23a to 0a4d5e7 Compare October 29, 2024 21:12
@Timmmm Timmmm mentioned this pull request Oct 29, 2024
parse.py Show resolved Hide resolved
@Timmmm
Copy link
Contributor Author

Timmmm commented Oct 30, 2024

I fixed the two grammar mistakes in the README you noticed.

Previously this generate a `instr_dict.yaml` file, however there is no need to use YAML here. The only advantages of YAML over JSON are that it is (debatably) easier for humans to write, making it suitable as human-edited input files. This is a machine-generated output file so JSON is better.

The main advantages are:

1. It removes the dependency on PyYAML, which is the only external dependency of this project. Python external dependencies are quite a pain - PyYAML is a particularly troublesome one - and having no dependencies is very convenient (no need for venv etc.).
2. It means consumers of the file don't need to depend on PyYAML.

Note this could have been done in an 100% backwards compatible way by keeping the file name unchanged (since JSON is valid YAML), however I figured there probably aren't that many users of this file, and since the only thing they need to change is the filename it's probably better to minimise confusion by renaming it. It also makes it clear that it is guaranteed to be JSON.
@aswaterman aswaterman merged commit bd5e598 into riscv:master Oct 31, 2024
2 checks passed
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.

4 participants