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

Propagate information about partition() part to children() #1480

Closed
wants to merge 2 commits into from

Conversation

zouppen
Copy link
Contributor

@zouppen zouppen commented Sep 29, 2024

This patch allows to customize the partitioned parts. The part is available in $partition_part variable and is either FIRST or SECOND).

This patch allows to customize the partitioned parts. The part is
available in $partition_part variable and is either FIRST or SECOND).
@adrianVmariano
Copy link
Collaborator

adrianVmariano commented Oct 3, 2024

Probably the values should be 0 and 1 instead of FIRST or SECOND. I also wonder if it should be $idx or $partition_idx. Should we rename some of the other $idx variables to $<module>_idx to keep them from stepping on each other?

@zouppen
Copy link
Contributor Author

zouppen commented Oct 3, 2024

One of the hard things in computing, whether to start from 0 or 1. My initial thought was that if we ever have BOTH (or ALL) then it could be zero, for example in case we'd add an argument like partition(keep=BOTH|FIRST|SECOND).

About the prefixing question: I have no preference since I'm not too familiar with your coding conventions. Of course non-colliding variable names are better than colliding :)

@adrianVmariano
Copy link
Collaborator

In a whole bunch of places modules propagate $idx with a zero base to refer to iterates of the module. See for example everything in distributors.scad. So the simplest more clearly consistent behavior would be to do the same. Doing $partition_idx could, like I said, keep things more separated, though probably a collision is unlikely in this case. Is "both" likely? I have no idea. I do not like introducing FIRST and SECOND which seem like needless namespace clutter.

@zouppen
Copy link
Contributor Author

zouppen commented Oct 3, 2024

I also have mixed feelings about them, maybe my history on statically typed languages led me to introduce them. Should we use a boolean instead? I can fix the patch, but I'd like to have your opinion about naming and typing, like:

  • $partition_is_first (true/false)
  • $partition_idx (0 or 1)
  • something else?

@adrianVmariano
Copy link
Collaborator

Do you think it is likely that someone would want to condition on which partition within the code inside other stuff, like grid() or xcopies() or the like? If this seems very unlikely I think maybe using $idx set to 0/1 would be best because it follows a pattern used in many other modules.

If we think collision is more likely then I like $partition_idx.

@zouppen
Copy link
Contributor Author

zouppen commented Oct 3, 2024

Hmm, maybe then I'd say it's best to go with the current convention. The collision is rather easy to avoid in user code by doing something like $partition_idx = $idx right inside the nested block before xcopies() or the like, if they need the partition index all the way down.

I'll prepare the patch tomorrow. :)

@adrianVmariano
Copy link
Collaborator

Yes, exactly. That's why I'd only deviate from that convention if it seems like a common or likely collision situation. So I think going with $idx set to 0/1 makes sense.

@revarbat
Copy link
Collaborator

revarbat commented Oct 4, 2024

Agreed with using $idx with values of 0 and 1 instead of constants.

@zouppen
Copy link
Contributor Author

zouppen commented Oct 6, 2024

GitHub has lost track of my commit for this pull request, maybe because I've done both rebase and force-with-lease to my branch. I close this and open a new one #1487

@zouppen zouppen closed this Oct 6, 2024
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.

3 participants