PR: Add universal screw parameters to screws_tilt_adjust with backward compatibility#844
Conversation
|
Hi @dalegaard , thanks a lot for the comments you left I agree with everything you pointed out. In the meantime, I’ll address the issues you mentioned. If you have any other suggestions or feedback, I’m more than happy to hear them. Thanks again for your feedback! |
Co-authored-by: dalegaard <dalegaard@gmail.com>
| ) | ||
| # Screw parameters: support both legacy 'screw_thread' and | ||
| # universal 'screw_pitch'/'screw_direction' options. | ||
| screw_thread = config.get("screw_thread", None) |
There was a problem hiding this comment.
Use getchoice for screw_thread and screw_direction so you don't need to do lookups later.
There was a problem hiding this comment.
I looked into using getchoice for screw_thread and screw_direction. It works well for screw_direction (already done in the else branch), but for screw_thread there are a few issues:
-
Case sensitivity: the previous code did
.upper()before lookup, socw-m3was accepted. getchoice does a case-sensitive comparison against dictionary keys, so lowercase input would break. We'd need to either add lowercase keys toSCREW_THREAD_MAPor accept this as a breaking change. -
Error messages:
getchoiceraises a generic"Choice '...' is not a valid choice"message, losing our custom error text. This also breaks existing test assertions that match on specific error strings. -
Validation order: when
screw_threadis not specified anddefault=None,getchoiceraises immediately becauseNoneis not a valid choice — before we can reach our XOR logic that checks whether the user provided the right combination of options.
There was a problem hiding this comment.
I think we have 3 possible approaches to the problem:
-
Use getchoice only when the option is actually present (check with config.get("screw_thread", None) first, then call getchoice), but that defeats the purpose of simplification.
-
Add both upper and lowercase keys to
SCREW_THREAD_MAPto preserve case insensitivity, and accept the generic error message. -
Keep the current manual approach for
screw_threadsince it handles a tuple return value and custom validation, and only use getchoice forscrew_direction(which is already done).
How do you think it's best to proceed?
There was a problem hiding this comment.
The original code was using getchoice for the screw_thread parameter so there shouldn't be a breaking change here. Likewise, there shouldn't be any tests broken by this.
You can add None: None to SCREW_THREAD_MAP to make None pass as a valid choice.
You're right that the error message is generic, but since we use this all across the codebase it's better to use getchoice here, and then in a separate patch extend getchoice so it will output valid options when it makes sense. That way, we fix that issue across the entire codebase.
Both screw_thread and screw_direction should be using getchoice at the initial lookup, instead of doing two lookups.
| if not ( | ||
| (screw_thread is not None) | ||
| ^ (screw_pitch is not None or screw_direction is not None) | ||
| ): |
There was a problem hiding this comment.
I know I suggested this one, but actually think it would be cleaner like:
has_screw_thread = screw_thread is not None
has screw_pitch = screw_pitch is not None or screw_direction is not None
if has_screw_thread == has_screw_pitch:Decoding the XOR with the NOT in front is a lot harder.
This PR makes the
screws_tilt_adjustmodule universal by allowing any screw thread pitch, instead of being limited to predefined screw types.Previously, only specific screw types (
M3,M4,M5,M6,M8) could be used via thescrew_threadparameter.What Changed
New Universal Parameters
screw_factor– screw thread pitch in millimetersscrew_direction– rotation direction (CW/CCW)Backward Compatibility
screw_threadparameter continues to work unchangedSimplified Implementation
Documentation Updated
Config_Reference.mdWhy
The old implementation was restrictive: users with custom screw sizes (e.g.
M10,M12, or non-standard thread pitches) could not use the tool.This change enables universal screw support while maintaining full backward compatibility with all existing configurations.
Example Usage
Checklist