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

"I'm a mechanical engineer, not a programmer" #10

Open
xkortex opened this issue Jan 19, 2020 · 3 comments
Open

"I'm a mechanical engineer, not a programmer" #10

xkortex opened this issue Jan 19, 2020 · 3 comments

Comments

@xkortex
Copy link

xkortex commented Jan 19, 2020

Yet you manage to write some of the cleanest, best-annotated Python code I've ever read. Bravo!

I love the type hints and the lack of external dependencies.

A couple of small suggestions which are more style/approach opinions than anything else:

  • In the main loop, you might consider making the main processing function which expects iter(lines_of_gcode) and returns (or better yet yields) lines as an iterator. Move the file i/o to another function which takes the input file, iterates the lines into the processor, and writes out. The thinking here is to make it easier to eventually integrate into a slicer/gcode pipeline
  • I see that there are some comments for syntax using F-strings - IMHO stick with .format for this script and target py3.5-3.7 for compatibility
  • Personally, I would parse and validate a line of gcode/comment in a single spot, rather than e.g. #L265 checking if "E" in the string in multiple places. Here's an example regex for parsing gcode. There are python gcode parsers out there, but I like the lack of deps, pip can be daunting for the general audience. No need to get crazy with classes, just a namedtuple or class with slots to contain the values of the fields, or None if not present. Or a dict where keys are letter codes.
  • I'd give process_gcode() some sane default parameters
  • Add an examples folder with known input/output pairs so contributors can validate their output

Again, excellent work!

@CNCKitchen
Copy link
Owner

The clean code is not due to my skills but due to the wonderful contribution of a couple of other skilled contributors.
I'll see if I can implement your suggestions!

@edlane
Copy link

edlane commented Jan 19, 2020

I agree with @xkortex
I am also a professional software engineer with 40 years experience ( last 7 years in Python ) and would find the code you originally submitted a delight to review -- good work!

@beeb
Copy link
Contributor

beeb commented Jan 23, 2020

Sorry about the f-strings, that's my bad! I agree with the remarks, if I get some time in the next week I might try a PR

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

No branches or pull requests

4 participants