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

Refactorings #38

Open
1 of 4 tasks
giannissc opened this issue May 8, 2023 · 5 comments
Open
1 of 4 tasks

Refactorings #38

giannissc opened this issue May 8, 2023 · 5 comments
Labels
quality Achievement unlocked: Shoe polish

Comments

@giannissc
Copy link
Contributor

giannissc commented May 8, 2023

  • There are a couple of NumPy deprecation warning when running pytestdue to typing. Should we do anything about them?
  • Are there any other class attributes to be converted to class properties (like you did for tool_radius)?
  • Should i change update_tool to use shallow copies for self = self._add_operation("Tool Change", commands)?
  • Remove unitest usages and refactor to pytest
@giannissc
Copy link
Contributor Author

This are just a couple of inconsistencies I noticed and wanted to tackle next. Feel free to add your own to track the changes that are needed to be made. Also can you please clarify which style is the preferred in the first and last points? @voneiden

@voneiden
Copy link
Owner

voneiden commented May 8, 2023

First) PEP 484 reads merely "As a shorthand for Union[T1, None] you can write Optional[T1]", so there's no official preference for either. SonarCloud does complain about Union[T1, None] though suggesting to replace it with union type expression T1 | None . I've not used these much, but seems also like a fairly reasonable option as it's shorter to type and doesn't require imports.

Not sure which one you refer to the last one as there have been a few edits.

There are a couple of NumPy deprecation warning when running pytestdue to typing. Should we do anything about them?

Is that covered by 5d8aec3 ?

Drill and Surface3D are classes but profile and pocket are method. Should we be consistent?

Yes, the class based is older structure and needs refactoring. Both were quite hastily bolted on the fluent api.

Should i change update_tool to use shallow copies for self = self._add_operation("Tool Change", commands)?

The self override in the middle of the method is certainly a bit unconventional and would be neat if there's some other way to handle this.

Some methods use typing information and some don't. Should we add to the rest as well?

Not a high priority, but at least I like having them.

@voneiden
Copy link
Owner

voneiden commented May 8, 2023

For priority, the to_gcode overhaul could be pretty high on the list. One large inconsistency that exists is the precision (gcode_precision). There are multiple places all over that require rounding (gcode output, spline approximation, clipper offsetting) of values, and currently it's all hardcoded with placeholder values. Would be nice to get that sorted, but it's probably best to finalize it after the to_gcode overhaul is done.

@giannissc
Copy link
Contributor Author

Is that covered by 5d8aec3 ?

I think so. I will keep it on the list to check later as well. I will break this into separate issues for different groups

@giannissc
Copy link
Contributor Author

New issues: #39, #40, #41, #42

@voneiden voneiden added the quality Achievement unlocked: Shoe polish label May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality Achievement unlocked: Shoe polish
Projects
None yet
Development

No branches or pull requests

2 participants