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

Local and Parameter Blocks #1494

Merged
merged 9 commits into from
Oct 22, 2024
Merged

Conversation

goetzrrGit
Copy link
Contributor

@goetzrrGit goetzrrGit commented Oct 8, 2024

Closes #1460
Adding new directives to the SeqN Grammer to support Parameters and Locals blocks.

  • New Directives: The addition of @INPUT_PARAMETER_BEGIN/END and @LOCALS_BEGIN/END directives provides more flexibility and structure for defining Local and Parameters.
  • Linter Updates: The linter has been updated to support the new grammar rules
  • SeqJSON Export and Import: Both the SeqJSON exporter and importer have been enhanced to fully support SeqJSON Local and Parameters spec.
  • Autocomplete: The autocomplete feature has been extended to include the new directives, improving developer productivity.

Testing:
You should be able to use the old grammer

@INPUT_PARAMS L00STR {"allowable_values": ["hi"]} TEMP {"type" : "UINT"}

@LOCALS L001UNIT L001ENUM

As well as the new grammar. The new grammar should be collapsable.

@LOCALS_BEGIN
TEMPERATURE INT
SIZE INT "-1...20, 40..."
STORE ENUM STORE_NAME "" "MACY, ROSS, BEST_BUY"
CHARGE
@LOCALS_END

@INPUT_PARAMS_BEGIN
DOOR_ENABLED INT
X_RANGE INT "...10" "-1,0,3,4,5,9"
@INPUT_PARAMS_END

Verify round-tripping works by exporting a seqJson and reimporting it and verify that the Locals and Input_params are correct.

@goetzrrGit goetzrrGit requested a review from a team as a code owner October 8, 2024 23:23
@goetzrrGit goetzrrGit added the sequencing Anything related to the sequencing domain label Oct 8, 2024
@goetzrrGit goetzrrGit force-pushed the feature/local_parameter_blocks branch from 2776a8a to 522e146 Compare October 8, 2024 23:25
@goetzrrGit goetzrrGit force-pushed the feature/local_parameter_blocks branch from 522e146 to e1db224 Compare October 17, 2024 18:34
@goetzrrGit goetzrrGit force-pushed the feature/local_parameter_blocks branch from e1db224 to 8404c92 Compare October 21, 2024 17:16
@goetzrrGit goetzrrGit force-pushed the feature/local_parameter_blocks branch from 8404c92 to 8f6ef3a Compare October 21, 2024 17:20
Copy link
Collaborator

@joswig joswig left a comment

Choose a reason for hiding this comment

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

I'd like to fix the linter typo as that was unclear when I was testing, the others are style and just my opinion.

Functionally it works well, my testing focused on folding and seq.json inspection.

I'll note that even when @LOCALS_BEGIN is present, the content assist can suggest an @Locals directive which will lint as an error. This is a minor issue and neither unique nor new, but I'd like to see how we can reduce on completion suggestion set to valid options. This may be impractical since we support custom linting

@goetzrrGit
Copy link
Contributor Author

I'd like to fix the linter typo as that was unclear when I was testing, the others are style and just my opinion.

Functionally it works well, my testing focused on folding and seq.json inspection.

I'll note that even when @LOCALS_BEGIN is present, the content assist can suggest an @Locals directive which will lint as an error. This is a minor issue and neither unique nor new, but I'd like to see how we can reduce on completion suggestion set to valid options. This may be impractical since we support custom linting

Thank you for the review. I fixed the issues you listed in the PR.

We can beef up the autocompletion further and should be okay with custom linting as you can modify/override the autocompletion within the adaptation file

@goetzrrGit goetzrrGit force-pushed the feature/local_parameter_blocks branch from b266379 to 4805ebc Compare October 22, 2024 18:28
* Added new directive @INPUT_PARAMETER_BEGIN/END and @LOCALS_BEGIN/END
* Checking both the new and old Parameters @INPUT_PARAMS and @INPUT_PARAMS_BEGIN

* Checking both the new and old Locals @Locals and @LOCALS_BEGIN
* new LOCAL and INPUT_PARAMS blocks
* SeqJson export of Local and Parameters are fully supported now
* Update the unit test as well
* new LOCAL and INPUT_PARAMS blocks
* Import full SeqJSON spec for Locals and Parameters
* Update the unit test as well
@goetzrrGit goetzrrGit force-pushed the feature/local_parameter_blocks branch from 4805ebc to 236a511 Compare October 22, 2024 18:29
Copy link
Collaborator

@joswig joswig left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback, I'm good merging when @duranb and @goetzrrGit are in agreement on the other items

@goetzrrGit goetzrrGit force-pushed the feature/local_parameter_blocks branch from 236a511 to f547fe9 Compare October 22, 2024 20:17
Copy link
Collaborator

@duranb duranb left a comment

Choose a reason for hiding this comment

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

lgtm!

@goetzrrGit goetzrrGit merged commit 8b0bd84 into develop Oct 22, 2024
5 checks passed
@goetzrrGit goetzrrGit deleted the feature/local_parameter_blocks branch October 22, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sequencing Anything related to the sequencing domain
Projects
None yet
4 participants