refactor(PD): ThreadSize enum simplification #18682
Conversation
github-actions
bot
added
the
Mod: Part Design
|
@AjinkyaDahale is going to look at this. |
| @@ -194,6 +194,44 @@ def testRefineHole(self): | |||
| self.Doc.recompute() | |||
| self.assertEqual(len(self.Hole.Shape.Faces), 7) | |||
|
|
|||
| def testThreadTypeChange(self): | |||
There was a problem hiding this comment.
I suggest to split this test in two, each with one specific purpose:
- For this PR:
testThreadTypeChangeChangesAvailableThreadSizes - For your other PR feat(PD): Auto find closest size when changing thread standards #18779:
testThreadTypeChangeFindsClosedThreadSize
As you refactored the way the enum values are determined and set, and there hasn't been a test for this previously, it would be good to address this first. You can use the getEnumerationsOfProperty method to get the possible values. E.g.
self.Hole.ThreadType = 'ISOMetricProfile'
allowed_sizes = self.Hole.getEnumerationsOfProperty("ThreadSize")
# then asserts on allowed_sizes; I'd say some spot checking is enoughI think we also should only set enums using their string representation (i.e. self.Hole.ThreadSize = 'M4' instead of self.Hole.ThreadSize = 11). I acknowledge that this source file currently uses a wild mix of both, but in my opinion new code should not copy the integer way.
Generally, long test methods of the form: act, assert, act, assert, act, assert, ... are considered an anti-pattern. Unfortunately, Python's unittest module does not support data driven tests for cases like the one here. But we can still improve the readability and reporting (in case of failures or errors) using sub-tests. See here for an example.
|
I think this is a very welcomed refactoring! I'm also happy to see that you extended the test suite as well. I compiled this branch and did a few rudimentary tests using the GUI and it all seems to work as expected. I'm no expert on modern C++, though, so I cannot comment on that part. Then I have a question regarding the order of the enum values. @alfrix do you know whether the new code produces the same order as the old one? (at a first glance it does) @chennes do you know whether the order of the enums matters for backward compatibility with existing files? |
| for (const auto& thread : Hole::threadDescription[threadType]) { | ||
| if (thread.designation != nullptr) { | ||
| designations.emplace_back(thread.designation); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can be replaced with std::transform and then std::erase(std::remove(...),...). Also, is the null check necessary?
Thank you for the feedback, i made the test to check that the enum order wans't changed, and both ThreadSize and ThreadType map correctly to the expected values. Setting enums using their string representation would defeat this purpose. I’m not seeing a significantly better way to structure this.
changing it to a vector would be more appropriate? -const Hole::ThreadDescription Hole::threadDescription[][171] =
+const std::vector<Hole::ThreadDescription> Hole::threadDescription[] =
i would prefer to avoid std::transform, is too obscure for coders that aren't focused on c++. changing to a vector guarantees non nulls, so yes it can be avoided if the type is changed: - if (thread.designation != nullptr) {
- designations.emplace_back(thread.designation);
- }
+ designations.push_back(thread.designation); |
Now that I think about it, this is much better. I'm not sure if the empty values earlier would have been initiated to
After removing the check, probably good enough. |
refactor to get the designations from the ThreadDescription instead of having a bunch of arrays duplicating the same thing
also added a test for threadtype change to check that is working properly