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

Add 'relative: bool' to tanArcTo, remove 'to' mode from tanArc #3705

Closed
wants to merge 12 commits into from

Conversation

adamchalmers
Copy link
Collaborator

Fixes #3319

Copy link

qa-wolf bot commented Aug 28, 2024

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link

vercel bot commented Aug 28, 2024

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

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Aug 29, 2024 7:28pm

@jtran
Copy link
Contributor

jtran commented Aug 28, 2024

Pretty sure these need to get updated.

getTag: getTag(),
addTag: addTag(),

The argument defaults to 2, but this PR adds a new parameter. So I think it needs to change to 3. It's which parameter is possibly the TagDeclarator.

@adamchalmers adamchalmers force-pushed the achalmers/again-fucking-tan-rebased branch 2 times, most recently from 3488086 to 3af8b57 Compare August 28, 2024 21:31
@jtran
Copy link
Contributor

jtran commented Aug 28, 2024

Sorry, I meant when we call getTag() and addTag() on these lines, the argument should be 3. The default of the functions shouldn't change.

  getTag: getTag(3),
  addTag: addTag(3),

getTag: getTag(),
addTag: addTag(),

@adamchalmers adamchalmers force-pushed the achalmers/again-fucking-tan-rebased branch from f5f72f7 to 9a51e8c Compare August 29, 2024 19:17
@@ -309,7 +309,7 @@ function singleRawValueHelper(
]
}

function getTag(index = 2): SketchLineHelper['getTag'] {
function getTag(index = 3): SketchLineHelper['getTag'] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I wasn't clear. I think we want to revert this change because it applies to all AST functions, not just tanArcTo.

Suggested change
function getTag(index = 3): SketchLineHelper['getTag'] {
function getTag(index = 2): SketchLineHelper['getTag'] {

@@ -1860,7 +1861,7 @@ function isAngleLiteral(lineArugement: Expr): boolean {

type addTagFn = (a: AddTagInfo) => { modifiedAst: Program; tag: string } | Error

function addTag(tagIndex = 2): addTagFn {
function addTag(tagIndex = 3): addTagFn {
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert here also.

Suggested change
function addTag(tagIndex = 3): addTagFn {
function addTag(tagIndex = 2): addTagFn {

@@ -849,8 +850,8 @@ export const tangentialArcTo: SketchLineHelper = {
pathToNode,
Copy link
Contributor

Choose a reason for hiding this comment

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

Above and below here are AST mod stuff specific to tangentialArcTo, so I suspect we need to change more stuff in this block.

@adamchalmers
Copy link
Collaborator Author

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.

tangentialArc should not have a "to" mode
2 participants