Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

dimension fix #298

Merged
merged 1 commit into from
Mar 14, 2019
Merged

dimension fix #298

merged 1 commit into from
Mar 14, 2019

Conversation

balcer
Copy link

@balcer balcer commented Feb 23, 2019

No description provided.

@codeclimate
Copy link

codeclimate bot commented Feb 23, 2019

Code Climate has analyzed commit 1273b87 and detected 0 issues on this pull request.

View more on Code Climate.

@evanshultz evanshultz mentioned this pull request Feb 25, 2019
5 tasks
@evanshultz
Copy link
Collaborator

A few comments from looking at your code, not running it, so check everything:

  1. You're not using the math or argparse or yaml modules so no need to include them. Using them would be nice, however, as most of our newer scripts do.
  2. The use or itr like this is a bit weird, but since there are two different connectors with pin count = 2 you will have to do something. One more elegant option would be to feed your generator script with the pin counts and other unique component information instead of embedded this inside the script, perhaps using the argparse module.
  3. 0.11 is a magic number here, but it's the silk offset and used in a number of places. Make that a variable.
  4. courtyard_outline is actually the courtyard offset.
  5. Lines 60, 63, and 64 should be able to be removed since they're defined at the top of the script, with line 60 being overwritten in the special case of MKS1652 on line 59. However, this entire PR is built around adding lines 63 and 64. I don't quite get it. Why aren't lines 37 and 44 making these three lines unneeded? And why is the footprint on line 55 being generated correctly if it doesn't set the value of fab_outline_x and silks_outline_x? Something is fishy here. Again, I didn't play around with it myself but there's something going on that isn't straightforward.
  6. If you use a pad array instead of Pad() on lines 75 and 80 it will automatically make pin 1 rect shape. This was a suggestion given in the original footprint submission and is still a valid improvement to your script.

@pointhi pointhi merged commit 49ea7e2 into pointhi:master Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants