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

Add support for TF command (FileAttribute) from GerberX2 #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shane-circuithub
Copy link

No description provided.

@shane-circuithub
Copy link
Author

This is completely untested right now, but I'm interested to see if either of you have any comments on the work so far before I go any further with it.

Copy link
Contributor

@HanStolpo HanStolpo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good and clean great work, I only have question about using the TH constructor for "NPTH" attributes which I think should change, and then maybe we can add a round trip test to see that we can parse what we print, that is if we have a print for the gerber commands.

parseVia :: MonadFail m => Field -> m Via
parseVia field = case field of
"PTH" -> pure TH
"NPTH" -> pure TH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we have a distinct constructor for non plated through hole, I feel we are loosing vital information here, also you won't be able to print what you parse

Copy link
Author

@shane-circuithub shane-circuithub Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I drop that information because it's redundant, and it allows me to use the same record for both the fields of both Plated and NonPlated. Here's the relevant part of the spec

§5.4.1.2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@HanStolpo
Copy link
Contributor

I see we don't have a printGerber to go with the parseGerber, so ignore my comment about a round trip test, we should probably add that in a future enhancement, especially when we want to convert Excellon Drill files to Gerber X2 files at a later stage.

Copy link
Contributor

@ocharles ocharles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All seems good to me! Just one suggestion - and I also agree with Handre's comment.

Comment on lines +28 to +29
(bytes, "") | ByteString.length bytes == 16 -> pure (MD5 bytes)
_ -> fail "Bad .MD5: must consist of exactly 32 hexadecimal digits"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You aren't actually checking both conditions of this error, which I think might lead to confusion later. I suggest we either do check that the digits are hexadecimal, or just drop this from the error message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am checking the condition. If they weren't hexadecimal digits then decode (from base16-bytestring) would fail.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least wouldn't return 16 bytes of data with nothing leftover.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I missed the call to decode. Great!

@ocharles
Copy link
Contributor

I see we don't have a printGerber to go with the parseGerber, so ignore my comment about a round trip test, we should probably add that in a future enhancement, especially when we want to convert Excellon Drill files to Gerber X2 files at a later stage.

I wouldn't ignore this comment, it's a fundamental architectural choice of the parser to try and faithfully parse the input into a data structure as close as possible to the input.

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.

3 participants