Fix RangeLiteral emitting spurious :1 for two-argument ranges#18
Open
tbjers wants to merge 2 commits into
Open
Fix RangeLiteral emitting spurious :1 for two-argument ranges#18tbjers wants to merge 2 commits into
tbjers wants to merge 2 commits into
Conversation
The builder synthesises a default step (NumberLiteral 1.0) tagged with the range node's own position when the source form is [start:end]. RangeLiteral.__str__ previously always emitted [start:end:step], turning [0:5] into [0:5:1]. OpenSCAD's three-argument syntax is [start:step:end], so [0:5:1] is read as start=0, step=5, end=1 — semantically opposite. Fix __str__ to compare step.position against the range node's position: same position means synthesised default → emit [start:end]; distinct position means an explicit third argument from source → emit [start:end:step]. Updates test_nodes.py to cover both the 2-arg and 3-arg cases with appropriate positions, and adds round-trip regression tests in test_vectors.py. Corrects two test_pretty_print assertions that were hard-coded to the old broken output (:1]). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
revarbat
reviewed
Jun 9, 2026
| step_pos = Position(origin="<test>", line=1, column=5) | ||
| step_explicit = NumberLiteral(val=2.0, position=step_pos) | ||
| range_3arg = RangeLiteral(start=_num(0.0), end=_num(5.0), step=step_explicit, position=_pos()) | ||
| assert str(range_3arg) == "[0:5:2]" |
Author
There was a problem hiding this comment.
Only if we are to fix it across the whole library, but that would be a breaking change, and would most likely warrant a major version bump. What do you think?
revarbat
reviewed
Jun 9, 2026
| # source position. | ||
| if self.step.position == self.position: | ||
| return f"[{self.start}:{self.end}]" | ||
| return f"[{self.start}:{self.end}:{self.step}]" |
Member
There was a problem hiding this comment.
This should probably be f"[{self.start}:{self.step}:{self.end}]"
Author
There was a problem hiding this comment.
That would require a much larger change across the library. I can do that if you would like, but it would become a breaking change in that case so you'd most likely need a major version bump?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The builder synthesises a default step (
NumberLiteral 1.0) tagged with the range node's own position when the source form is[start:end].RangeLiteral.__str__previously always emitted[start:end:step], turning[0:5]into[0:5:1]. OpenSCAD's three-argument syntax is[start:step:end], so[0:5:1]is read as start=0, step=5, end=1 — semantically opposite.Fix
__str__to comparestep.positionagainst the range node's position: same position means synthesised default → emit[start:end]; distinct position means an explicit third argument from source → emit[start:end:step].Updates
test_nodes.pyto cover both the 2-arg and 3-arg cases with appropriate positions, and adds round-trip regression tests intest_vectors.py. Corrects twotest_pretty_printassertions that were hard-coded to the old broken output (:1]).