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

Repeated structured type declaration fails to parse #105

Merged
merged 11 commits into from
Jan 31, 2025

Conversation

engineerjoe440
Copy link
Collaborator

No description provided.

@engineerjoe440 engineerjoe440 self-assigned this Nov 27, 2024
Base automatically changed from enhancement/enum-with-functions-for-base-values to master December 5, 2024 00:45
@klauer
Copy link
Owner

klauer commented Dec 18, 2024

This one going OK, or would you like me to take a look?

@engineerjoe440
Copy link
Collaborator Author

Feel free to jump in! I haven't had the opportunity to spend much time on this, recently. I was seemingly running into some challenges, but I can't recall (now) what those were. If you don't get a chance, don't worry about it, I'll poke around at this some more as time permits. Thank you!

@dfreiberger
Copy link

@engineerjoe440 is this just waiting on the tests to be fixed? I ran into the same issue and found this PR while trying to fix it. I was able to use the code in this PR to get through the issue.

@klauer
Copy link
Owner

klauer commented Jan 25, 2025

I think/hope that should do it - tests are passing locally.

Mind giving the updated changes a spin @engineerjoe440 and @dfreiberger?

@klauer klauer marked this pull request as ready for review January 25, 2025 20:16
@dfreiberger
Copy link

I ran the tests locally and they passed. I tested against a couple of PLC structures previously failing due to repeated variables and it now parses these correctly. Approving PR.

Copy link

@dfreiberger dfreiberger left a comment

Choose a reason for hiding this comment

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

In test_transformer.py/test_expression_roundtrip there was an additional test added in https://github.com/klauer/blark/blob/efd3a57afa475229f53ae9eddabdb75efb794dde/blark/tests/test_transformer.py and then later removed: param("simple_type_declaration", "TypeName, TypeName2 : INT"),

Should we add this back in as param("structure_element_declaration", "TypeName, TypeName2 : INT"), to extend coverage? Or is this already covered by the inclusion of repeated_declaration.st?

@klauer
Copy link
Owner

klauer commented Jan 30, 2025

Should we add this back in as param("structure_element_declaration", "TypeName, TypeName2 : INT"), to extend coverage? Or is this already covered by the inclusion of repeated_declaration.st?

The grammar modification is technically covered by the additional .st file in the test suite, but an explicit check as part of the suite is a good addition (cbbbf7c). There's plenty of room for improvement there with other aspects of the grammar, too.

Thanks for giving blark a try and helping test this PR, @dfreiberger. I'll merge and tag this week if I don't hear back from @engineerjoe440.

@klauer klauer merged commit 672c469 into master Jan 31, 2025
38 checks passed
@klauer klauer deleted the repeated-structured-type-declaration-fails-to-parse branch January 31, 2025 16:43
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