Skip to content

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

Open

Description

@xkortex

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!

Activity

  1. CNCKitchen commented on Jan 19, 2020

    @CNCKitchen
    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!

  2. edlane commented on Jan 19, 2020

    @edlane

    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!

  3. beeb commented on Jan 23, 2020

    @beeb
    Contributor

    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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions