Argmaster / pygerber

Python implementation of Gerber X3/X2 standard with 2D rendering engine.
https://argmaster.github.io/pygerber/stable
MIT License
54 stars 10 forks source link

Add support for Altium implied leading zeros omitted #340

Closed sjgallagher2 closed 2 days ago

sjgallagher2 commented 3 days ago

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[bot] commented 3 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #340 +/- ## ======================================= Coverage 93.50% 93.50% ======================================= Files 149 149 Lines 6955 6957 +2 ======================================= + Hits 6503 6505 +2 Misses 452 452 ``` | [Flag](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | Coverage Δ | | |---|---|---| | [all/ubuntu-20.04/3.13](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.50% <100.00%> (+<0.01%)` | :arrow_up: | | [all/ubuntu-20.04/3.8](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.38% <100.00%> (+<0.01%)` | :arrow_up: | | [all/ubuntu-22.04/3.13](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.50% <100.00%> (+<0.01%)` | :arrow_up: | | [all/ubuntu-22.04/3.8](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.38% <100.00%> (+<0.01%)` | :arrow_up: | | [all/ubuntu-24.04/3.13](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.50% <100.00%> (+<0.01%)` | :arrow_up: | | [all/ubuntu-24.04/3.8](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.38% <100.00%> (+<0.01%)` | :arrow_up: | | [all/windows-2019/3.13](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.50% <100.00%> (+<0.01%)` | :arrow_up: | | [all/windows-2019/3.8](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.38% <100.00%> (+<0.01%)` | :arrow_up: | | [all/windows-2022/3.13](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.50% <100.00%> (+<0.01%)` | :arrow_up: | | [all/windows-2022/3.8](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `93.38% <100.00%> (+<0.01%)` | :arrow_up: | | [e2e/ubuntu-24.04/3.10](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `76.06% <100.00%> (+<0.01%)` | :arrow_up: | | [e2e/ubuntu-24.04/3.11](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `76.06% <100.00%> (+<0.01%)` | :arrow_up: | | [e2e/ubuntu-24.04/3.12](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `76.06% <100.00%> (+<0.01%)` | :arrow_up: | | [e2e/ubuntu-24.04/3.9](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `75.64% <100.00%> (+<0.01%)` | :arrow_up: | | [e2e/windows-2022/3.10](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `76.06% <100.00%> (+<0.01%)` | :arrow_up: | | [e2e/windows-2022/3.11](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `76.06% <100.00%> (+<0.01%)` | :arrow_up: | | [e2e/windows-2022/3.12](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `76.06% <100.00%> (+<0.01%)` | :arrow_up: | | [e2e/windows-2022/3.9](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `75.64% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-20.04/3.10](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-20.04/3.11](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-20.04/3.12](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-20.04/3.9](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `78.65% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-22.04/3.10](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-22.04/3.11](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-22.04/3.12](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-22.04/3.9](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `78.65% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-24.04/3.10](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-24.04/3.11](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-24.04/3.12](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/ubuntu-24.04/3.9](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `78.65% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/windows-2019/3.10](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/windows-2019/3.11](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/windows-2019/3.12](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/windows-2019/3.9](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `78.65% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/windows-2022/3.10](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/windows-2022/3.11](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/windows-2022/3.12](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `79.02% <100.00%> (+<0.01%)` | :arrow_up: | | [unit/windows-2022/3.9](https://app.codecov.io/gh/Argmaster/pygerber/pull/340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski) | `78.65% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Krzysztof+Wi%C5%9Bniewski#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

Argmaster commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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.

sjgallagher2 commented 3 days ago

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 commented 3 days ago

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.