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

Update JSON Dictionary spec #2663

Merged
merged 14 commits into from
Apr 15, 2024
Merged

Update JSON Dictionary spec #2663

merged 14 commits into from
Apr 15, 2024

Conversation

thomas-bc
Copy link
Collaborator

Related Issue(s)
Has Unit Tests (y/n)
Documentation Included (y/n)

Change Description

  • Renamed Type Name to Type Descriptor
  • Introduced Enumerated Constant Descriptor and Struct Member Descriptor for sub-dictionaries that describe each concept
  • Renamed description to annotation throughout in the spec and added it where it was missing
  • Clarified some of the Description/Option tables to avoid repetition
  • Moved Parameters section after Events to respect the CTEP (Commands>Telemetry>Events>Parameter) order

Rationale

Updating the spec with regards to feedback from inputting into the GDS / build system

Testing/Review Recommendations

Will be drastically easier to review with the "Display Rich Diff" option in the changed files
image

@thomas-bc thomas-bc requested a review from bocchino April 5, 2024 00:42
docs/Design/fpp-json-dict.md Fixed Show fixed Hide fixed
docs/Design/fpp-json-dict.md Fixed Show fixed Hide fixed
docs/Design/fpp-json-dict.md Fixed Show fixed Hide fixed
docs/Design/fpp-json-dict.md Fixed Show fixed Hide fixed
docs/Design/fpp-json-dict.md Fixed Show resolved Hide resolved
docs/Design/fpp-json-dict.md Fixed Show resolved Hide resolved
docs/Design/fpp-json-dict.md Fixed Show fixed Hide fixed
docs/Design/fpp-json-dict.md Fixed Show fixed Hide fixed
@bocchino
Copy link
Collaborator

bocchino commented Apr 5, 2024

FPP annotations must be ASCII character strings. You could replace Schrödinger with Schroedinger.

Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

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

Looks good! Just one comment. In the type descriptor for a string type, I think the size field should have the description "Maximum string size in bytes" and the field should be required.

@thomas-bc
Copy link
Collaborator Author

Agreed, fixed!

@thomas-bc
Copy link
Collaborator Author

Note to self: there's an issue with the rendering of some tables in the spec on the website: https://nasa.github.io/fprime/Design/fpp-json-dict.html

This could be the solution: https://github.com/orgs/community/discussions/23945#discussioncomment-3242285

@thomas-bc
Copy link
Collaborator Author

nvm: very likely has to do with whitespaces https://www.reddit.com/r/github/comments/15gexlw/comment/juik4y3

Fixed

Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

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

Looks good!

@thomas-bc
Copy link
Collaborator Author

This state of the JSON dictionary spec is in sync with fpp-to-dict=2.1.0a7 (https://pypi.org/project/fprime-fpp-to-dict/2.1.0a7/)

@thomas-bc thomas-bc merged commit c4fc9a9 into devel Apr 15, 2024
4 checks passed
@thomas-bc thomas-bc deleted the fpp-json-dict-spec branch April 15, 2024 16:23
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.

3 participants