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

[Any Drill][Fixed] Do not overwrite other drill pairs with pth_id #758

Merged
merged 2 commits into from
Jan 2, 2025

Conversation

nguyen-v
Copy link

@nguyen-v nguyen-v commented Jan 2, 2025

This PR fixes the overwriting of non through-hole drill pair files when pth_id is specified.
I've also modifed the YAML files for the test cases (non-legacy) so that the default pth_id and npth_id are explicitly specified. Without the fix, it will overwrite the files which results in the test failing.
I'm not sure how to modify the legacy YAML files for this test, I've tried to add pth_id and npth_id but doesn't seem to change anything, I assume the legacy way of generating drill files did not have a function to rename the files? In this case it would be a non-issue

@nguyen-v nguyen-v changed the title [Any Drill][Fixed] Do not overwrite inner drill pairs with pth_id [Any Drill][Fixed] Do not overwrite other drill pairs with pth_id Jan 2, 2025
@set-soft
Copy link
Member

set-soft commented Jan 2, 2025

I've also modifed the YAML files for the test cases (non-legacy) so that the default pth_id and npth_id are explicitly specified. Without the fix, it will overwrite the files which results in the test failing.

I don't think you need to modify the YAML files, please verify it, I did a quick check and these changes aren't needed.

@nguyen-v
Copy link
Author

nguyen-v commented Jan 2, 2025

I've also modifed the YAML files for the test cases (non-legacy) so that the default pth_id and npth_id are explicitly specified. Without the fix, it will overwrite the files which results in the test failing.

I don't think you need to modify the YAML files, please verify it, I did a quick check and these changes aren't needed.

With the modified YAML file, the tests fail without the fix and pass with the fix. Wouldn't we need to modify them to catch the overwriting case, or is fixing the root of the problem enough?

@set-soft
Copy link
Member

set-soft commented Jan 2, 2025

Then modify one of them, lets say drill_single so we get a mix

@set-soft set-soft merged commit 7dbe0fb into INTI-CMNB:dev Jan 2, 2025
17 checks passed
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.

2 participants