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

FIX: string length expression #72

Merged
merged 4 commits into from
Jul 3, 2023
Merged

Conversation

klauer
Copy link
Owner

@klauer klauer commented Jun 23, 2023

Context

Closes #70

Building on @engineerjoe440 's branch

Changes

Continuous Integration

  • CI will only test on Python 3.9+ going forward.
    • 3.7 and 3.8 will be considered unsupported - but some/most of blark may still work on them

Grammar

  • Added bracketed_expression used solely for string lengths as in STRING[255]
  • Grammar change - terminal STRING_TYPE -> is now a rule string_type_specification as it must support expressions for length and not just simple integers/constants/terminals
  • string_spec_length added to support both flavors of string type declaration lengths STRING[255] and STRING(255)

Dataclasses

  • Adjusted dataclasses to support the grammar changes
  • SimpleSpecification gets a bit more complicated as its type is no longer just a string Token

Test suite

  • Added a bunch of TwinCAT-approved string declarations, which went beyond what I expected to compile
  • Added some TwinCAT disapproved string declarations and commented them out, but perhaps they should be added and marked as strict xfail. blark might be more permissive here than it should be, though.

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2023

Codecov Report

Merging #72 (39c32d0) into master (1e09713) will increase coverage by 0.2%.
The diff coverage is 90.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #72     +/-   ##
========================================
+ Coverage    72.2%   72.5%   +0.2%     
========================================
  Files          18      18             
  Lines        3816    3835     +19     
========================================
+ Hits         2758    2783     +25     
+ Misses       1058    1052      -6     
Impacted Files Coverage Δ
blark/tests/conftest.py 83.6% <0.0%> (ø)
blark/transform.py 98.9% <92.0%> (-0.2%) ⬇️
blark/tests/test_transformer.py 94.5% <100.0%> (+0.1%) ⬆️

... and 2 files with indirect coverage changes

@klauer klauer marked this pull request as ready for review June 23, 2023 19:14
@klauer
Copy link
Owner Author

klauer commented Jun 26, 2023

@engineerjoe440 mind taking a look before I merge this?

Copy link
Collaborator

@engineerjoe440 engineerjoe440 left a comment

Choose a reason for hiding this comment

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

The changes all look good and seem clear, to me. I won't have a chance to run this up in test until Friday of this week (at the earliest), because I'm helping with a youth conference. But I still think this looks nice! 😃

@klauer
Copy link
Owner Author

klauer commented Jun 28, 2023

Ah, no rush - blark is low priority for all involved. :) If you're willing to take a look later I'm happy to wait to merge. I'll be out for about a week starting tomorrow, also.

Have fun with the conference!

@engineerjoe440
Copy link
Collaborator

Thank you! I'll give it a run-down on Friday. Can't wait!

@engineerjoe440
Copy link
Collaborator

Just gave this a quick run and it's looking pretty nice! Thank you, again!

@engineerjoe440 engineerjoe440 merged commit eaded83 into master Jul 3, 2023
@engineerjoe440 engineerjoe440 deleted the fix_string_length_expr branch July 3, 2023 22: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.

POINTER TO STRING(c_Size - 1); Not Parsing Correctly
3 participants