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

Make tag last optional param everywhere #1739

Merged
merged 19 commits into from
Mar 15, 2024
Merged

Conversation

iterion
Copy link
Contributor

@iterion iterion commented Mar 15, 2024

closes #1730

Copy link

vercel bot commented Mar 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Mar 15, 2024 8:52pm

@jessfraz
Copy link
Contributor

Awesome thanks for doing this lgtm when all tests pas cc @jgomez720 to update his samples everywhere and I can bump the cli after

@jgomez720
Copy link
Collaborator

Oh super cool. Just to make sure I'm understanding this right, this is changing the structure from

|> line({to: [5, 5], tag: 'mySuperNiceLine'}, %)

to

|> line([5, 5], %, 'mySuperNiceLine')

@iterion
Copy link
Contributor Author

iterion commented Mar 15, 2024

Yep, you got it! Anything that took an optional tag: "something" will change to the second format you gave where tag is always the optional last parameter.

@jessfraz
Copy link
Contributor

why isnt cargo test running?

@jessfraz
Copy link
Contributor

rebase on this once merged #1740

@iterion iterion merged commit 8168702 into main Mar 15, 2024
18 of 19 checks passed
@iterion iterion deleted the iterion/change-line-to-signature branch March 15, 2024 21:03
@Irev-Dev Irev-Dev mentioned this pull request Mar 17, 2024
@Irev-Dev
Copy link
Collaborator

FYI found a regression with this.
#1747

Nice work though 👏

@Irev-Dev
Copy link
Collaborator

Irev-Dev commented Mar 17, 2024

The bug I found was it adding the classic js object Object as a tag 😂

Screenshare.-.2024-03-17.5_18_52.PM.mp4

@iterion
Copy link
Contributor Author

iterion commented Mar 18, 2024

HA, of course I missed something! I even fixed a bug just like that one elsewhere. Thanks for the quick fix and explanation. I wonder if I can get some unit test coverage on that, the existing unit tests were invaluable in making this change.

@Irev-Dev
Copy link
Collaborator

Well I found it on my "sketchOnFace" branch after merging in main, because I did have extrude working back when it was my fake engine it's probably found some tag logic that hasn't been used in a long time. Probably means it was under tested before, but this might also be a timing thing that I happen to be working on sketchOnFace when this went through, so maybe less of a regression and more of a regression in my branch ya know.

I do have one e2e test for the sketch on face, I plan on adding more. I just added a add unit test for the extrusion codemod in my TODO list in the description.

@Irev-Dev
Copy link
Collaborator

SketchOnExtrudedFace unit test is in my branch now too

9129f9a

@pierremtb pierremtb mentioned this pull request Mar 19, 2024
@iterion
Copy link
Contributor Author

iterion commented Mar 19, 2024

Fantastic, thanks @Irev-Dev!

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.

lineTo tag should be consistent with tangentialArcTo and others
4 participants