Add support for Altium implied leading zeros omitted #340

sjgallagher2 · 2024-11-18T18:02:36Z

Altium generates files with format specification %FSAX...Y...% (no L for leading zero omission). This change updates the grammar to accept an empty Zeros parameter defaulting to lead zero omission.

codecov · 2024-11-18T18:53:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.50%. Comparing base (70357c7) to head (872fd5d).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #340   +/-   ##
=======================================
  Coverage   93.50%   93.50%           
=======================================
  Files         149      149           
  Lines        6955     6957    +2     
=======================================
+ Hits         6503     6505    +2     
  Misses        452      452

Flag	Coverage Δ
all/ubuntu-20.04/3.13	`93.50% <100.00%> (+<0.01%)`	⬆️
all/ubuntu-20.04/3.8	`93.38% <100.00%> (+<0.01%)`	⬆️
all/ubuntu-22.04/3.13	`93.50% <100.00%> (+<0.01%)`	⬆️
all/ubuntu-22.04/3.8	`93.38% <100.00%> (+<0.01%)`	⬆️
all/ubuntu-24.04/3.13	`93.50% <100.00%> (+<0.01%)`	⬆️
all/ubuntu-24.04/3.8	`93.38% <100.00%> (+<0.01%)`	⬆️
all/windows-2019/3.13	`93.50% <100.00%> (+<0.01%)`	⬆️
all/windows-2019/3.8	`93.38% <100.00%> (+<0.01%)`	⬆️
all/windows-2022/3.13	`93.50% <100.00%> (+<0.01%)`	⬆️
all/windows-2022/3.8	`93.38% <100.00%> (+<0.01%)`	⬆️
e2e/ubuntu-24.04/3.10	`76.06% <100.00%> (+<0.01%)`	⬆️
e2e/ubuntu-24.04/3.11	`76.06% <100.00%> (+<0.01%)`	⬆️
e2e/ubuntu-24.04/3.12	`76.06% <100.00%> (+<0.01%)`	⬆️
e2e/ubuntu-24.04/3.9	`75.64% <100.00%> (+<0.01%)`	⬆️
e2e/windows-2022/3.10	`76.06% <100.00%> (+<0.01%)`	⬆️
e2e/windows-2022/3.11	`76.06% <100.00%> (+<0.01%)`	⬆️
e2e/windows-2022/3.12	`76.06% <100.00%> (+<0.01%)`	⬆️
e2e/windows-2022/3.9	`75.64% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-20.04/3.10	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-20.04/3.11	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-20.04/3.12	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-20.04/3.9	`78.65% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-22.04/3.10	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-22.04/3.11	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-22.04/3.12	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-22.04/3.9	`78.65% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-24.04/3.10	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-24.04/3.11	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-24.04/3.12	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/ubuntu-24.04/3.9	`78.65% <100.00%> (+<0.01%)`	⬆️
unit/windows-2019/3.10	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/windows-2019/3.11	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/windows-2019/3.12	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/windows-2019/3.9	`78.65% <100.00%> (+<0.01%)`	⬆️
unit/windows-2022/3.10	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/windows-2022/3.11	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/windows-2022/3.12	`79.02% <100.00%> (+<0.01%)`	⬆️
unit/windows-2022/3.9	`78.65% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

Argmaster

Hi @sjgallagher2

Could you please include a test case which checks if this feature works correctly for purpose of regression testing?

.gitignore

Argmaster · 2024-11-18T20:51:12Z

Please also address issues reported by CI. You can reproduce them locally by running

poetry shell
poetry install
poe prepare-test-style
poe test-style

sjgallagher2 · 2024-11-19T14:26:09Z

Previously worked around using Poetry but now have everything sorted, fixed up the tree and removed the local/ directory from .gitignore, hook checks all pass, just need to do the test case.

I'm new to github and pull requests, is it easier to just close this PR and open a new one with a correct branch (implied-lead-2)? I worked there from origin/main to avoid having to undo things and get into git-fu. Or better to push changes right to this branch?

For the test, I made some random geometry and generated this gerber (zipped for github):
altium1.zip
How should I add a test for this? Add the altium test file to examples/ and the ExamplesEnum and add a new test to test/examples/gerberx3/api?

The test:

from pygerber.gerber.api import GerberFile, Style

from pygerber.examples import ExamplesEnum, get_example_path

path_to_gerber_file = #get_example_path(ExamplesEnum.Altium_01)   #<- depends on test file zipped above

image = GerberFile.from_file(path_to_gerber_file).render_with_shapely(
    Style.presets.BLACK_WHITE
)
image.save("output.svg")

Argmaster · 2024-11-19T15:13:27Z

Previously worked around using Poetry but now have everything sorted, fixed up the tree and removed the local/ directory from .gitignore, hook checks all pass, just need to do the test case.

Thanks

I'm new to github and pull requests, is it easier to just close this PR and open a new one with a correct branch (implied-lead-2)? I worked there from origin/main to avoid having to undo things and get into git-fu. Or better to push changes right to this branch?

The preferred way to handle updates for exisitng pull request is to keep the pull request open and contribute fixes to the same branch as initially. Github will automatically track changes on your branch and you will see them reflected by the PR. If you want to undo things via git use git revert command, avid hard resets and force pushses, this disrupts reviewing process. If you have changes on different branch you can either use git merge to merge branches or git cherry-pick the commits then push to feature/implied-leading. Avoid hard resetting branches which are being used by pull requests, as it disrupts reviewing process.

What I mean by discrupting reviewing process is that when you build on top of existing code, reviewer can skip parts of code which you didn't modify while complying with previous reviewer requests. If you force push a branch Github has tendency to forget which parts of code were marked as reviewed and reviewer will have to do all the work again.

Since I have merged main into your branch to enable CI to run, you will likely have to pull my changes from remote branch in your repository. If you have commited things to this branch without pulling changes from remote branch, use git fetch and git rebase origin/feature/implied-leading to align history of changes on your local branch.

For the test, I made some random geometry and generated this gerber (zipped for github): altium1.zip How should I add a test for this? Add the altium test file to examples/ and the ExamplesEnum and add a new test to test/examples/gerberx3/api?

The test:
from pygerber.gerber.api import GerberFile, Style

from pygerber.examples import ExamplesEnum, get_example_path

path_to_gerber_file = #get_example_path(ExamplesEnum.Altium_01)   #<- depends on test file zipped above

image = GerberFile.from_file(path_to_gerber_file).render_with_shapely(
    Style.presets.BLACK_WHITE
)
image.save("output.svg")

This feature is not exposed enough to make it necessary to include sample files which ilustrate it within PyGerber release.
Assets which are used only for testing are stored in test/assets/gerberx3 directory structure. In your case it would be a good idea to include example of FS node alone in file in test/assets/gerberx3/tokens/properties directory and a full Gerber file in
test/assets/gerberx3/issues/340/. Those Gerber asset files should be automatically discovered and basic parser tests should be created for them automatically. On top of those automatically created test cases it would be a good idea to add a test that verifies StateTrackingVisitor logic you have implemented. This should be done separately in test/unit/test_gerber/test_ast/test_state_tracking_visitor.py, there are already test cases which check behavior of FS node, you can use them for reference.

…erber into feature/implied-leading

sjgallagher2 · 2024-11-19T16:19:13Z

Thanks so much for the detailed help, I think I've got everything in order now, tests pass.

PS. I noticed that this page says poe run-unit-tests but I think it should be poe test-unit since the first didn't work for me. Worth opening a ticket?

Argmaster · 2024-11-19T16:37:48Z

Thanks so much for the detailed help, I think I've got everything in order now, tests pass.

Sure, no problem, we have to wait for the results on latest commit though.

PS. I noticed that this page says poe run-unit-tests but I think it should be poe test-unit since the first didn't work for me. Worth opening a ticket?

Well, It would have been worth opening a ticked, but I already updated contribution guidelines yesterday, you can find it here, on dev version.

Sam Gallagher and others added 4 commits November 18, 2024 07:34

Update gitignore

ba0c1e8

Add .idea to gitignore

d729d21

Update to allow implied leading zeros for Altium

b0fbf6f

Merge branch 'main' into feature/implied-leading

11b3890

Argmaster added the enhancement New feature or request label Nov 18, 2024

Argmaster added this to the 3.0.0a4 milestone Nov 18, 2024

Argmaster linked an issue Nov 18, 2024 that may be closed by this pull request

Pre-Release 3.0.0a4 #317

Closed

14 tasks

Argmaster mentioned this pull request Nov 18, 2024

Pre-Release 3.0.0a4 #317

Closed

14 tasks

Argmaster reviewed Nov 18, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Sam Gallagher and others added 5 commits November 19, 2024 10:41

Fix .gitignore and state_tracking_visitor.py style

6fa8df8

Merge branch 'feature/implied-leading' of github.com:sjgallagher2/pyg…

fc56000

…erber into feature/implied-leading

Merge branch 'main' into feature/implied-leading

c526dd5

Update tests for altium implied leading zero omission support

f7d4ac7

Merge branch 'feature/implied-leading' of github.com:sjgallagher2/pyg…

872fd5d

…erber into feature/implied-leading

Argmaster approved these changes Nov 19, 2024

View reviewed changes

Argmaster merged commit f39c223 into Argmaster:main Nov 19, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Altium implied leading zeros omitted #340

Add support for Altium implied leading zeros omitted #340

sjgallagher2 commented Nov 18, 2024

codecov bot commented Nov 18, 2024 •

edited

Loading

Argmaster left a comment •

edited

Loading

Argmaster commented Nov 18, 2024 •

edited

Loading

sjgallagher2 commented Nov 19, 2024

Argmaster commented Nov 19, 2024 •

edited

Loading

sjgallagher2 commented Nov 19, 2024

Argmaster commented Nov 19, 2024 •

edited

Loading

Add support for Altium implied leading zeros omitted #340

Add support for Altium implied leading zeros omitted #340

Conversation

sjgallagher2 commented Nov 18, 2024

codecov bot commented Nov 18, 2024 • edited Loading

Codecov Report

Argmaster left a comment • edited Loading

Choose a reason for hiding this comment

Argmaster commented Nov 18, 2024 • edited Loading

sjgallagher2 commented Nov 19, 2024

Argmaster commented Nov 19, 2024 • edited Loading

sjgallagher2 commented Nov 19, 2024

Argmaster commented Nov 19, 2024 • edited Loading

codecov bot commented Nov 18, 2024 •

edited

Loading

Argmaster left a comment •

edited

Loading

Argmaster commented Nov 18, 2024 •

edited

Loading

Argmaster commented Nov 19, 2024 •

edited

Loading

Argmaster commented Nov 19, 2024 •

edited

Loading