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

Cleanup #71

Merged
merged 13 commits into from
Mar 29, 2020
Merged

Cleanup #71

merged 13 commits into from
Mar 29, 2020

Conversation

Illusion65
Copy link

I replaced the deprecated child() and assign() in the library. I also made a few changes while testing/validating my updates:

  • applying an operation to all children, rather than only the first
  • updating some modules to work properly (or as I think they should work - check multiply.scad commit 87cf713)
  • also fixed deprecated warnings in regular_shapes.scad - see Issue List of Deprecation Warnings #69

I tried to make the changes clear in each commit comment.
Thanks for a great library!

array.scad Outdated Show resolved Hide resolved
Copy link

@hyperair hyperair left a comment

Choose a reason for hiding this comment

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

Looks good except for the few comments I left

array.scad Outdated Show resolved Hide resolved
array.scad Outdated Show resolved Hide resolved
@Illusion65
Copy link
Author

I just checked, the [0:$children-1] parameter is necessary for the center parameter to not become a syntax error (center cannot be the only parameter of children()). Odd.

Also, I parameterized to remove the if...else clause: 488b1ee

Final change for the day... Thanks!

for (i = [0 : $children-1]) child(i);
}
}
// Copy everything $no of times around an $axis, spread over $angle

Choose a reason for hiding this comment

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

Variables can begin with $ in openscad, so don't use $no, $axis, et. al. to denote the no and axis parameters; it'll cause confusion in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Good call. I saw that in multiply.scad and thought I was following this library's format for comments. I'll work on another pull request to fix those comments in other files.

gridbeam.scad Show resolved Hide resolved
array.scad Outdated Show resolved Hide resolved
array.scad Outdated Show resolved Hide resolved
involute_gears.scad Show resolved Hide resolved
involute_gears.scad Show resolved Hide resolved
@hyperair hyperair merged commit e2fa115 into openscad:master Mar 29, 2020
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