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

Gcode class #52

Closed
wants to merge 6 commits into from
Closed

Gcode class #52

wants to merge 6 commits into from

Conversation

philipp1604
Copy link
Collaborator

@philipp1604 philipp1604 commented Nov 30, 2023

Description

The following changes have been made to the GCodeProcessor:

  1. All names, methods, and variables have been uniformly changed to use the identifier g_code.
  2. The format of the g_code has been updated to an array dictionary format ([{G: 1, X: 2.0, Y: 1.0}, {G: 1, X: 1.0}, ...]).
  3. The read_g_code() method has been modified to convert the input to this new format.

Motivation and Context

Change Number 1 was implemented to enhance the overall code style.

Changes Number 1 and 3 were made to improve the handling of g_code. Utilizing dictionaries makes it more convenient to manage information within the G-code.

How has this been tested?

The changes have been thoroughly tested using the test method. The modifications are isolated and do not impact other areas of the code.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

- the GCode Processor was improved in many way
- usage of more dicionary methods
- g_code was changed to an array[dictionary] format
- this enables a better handling of the g_code inside and outside of the processor
- adjusting test method
- test class was adapted to dictionary format
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (fa11024) 85.27% compared to head (e16f2e6) 86.96%.

Files Patch % Lines
src/pybullet_industrial/g_code_processor.py 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   85.27%   86.96%   +1.68%     
==========================================
  Files          15       15              
  Lines         951      951              
==========================================
+ Hits          811      827      +16     
+ Misses        140      124      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- adding empty line
Copy link
Member

@liquidcronos liquidcronos left a comment

Choose a reason for hiding this comment

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

It is completely unclear to me what the changes proposed are meant to achieve.
It seems that some are just renaming variables while other change lists to dict etc...
Is it just a refractoring that does not change the behavior of the code or is this pull request fixing any bugs?
Please fill out the pull request template to clarify

@philipp1604
Copy link
Collaborator Author

It is completely unclear to me what the changes proposed are meant to achieve. It seems that some are just renaming variables while other change lists to dict etc... Is it just a refractoring that does not change the behavior of the code or is this pull request fixing any bugs? Please fill out the pull request template to clarify

It is completely unclear to me what the changes proposed are meant to achieve. It seems that some are just renaming variables while other change lists to dict etc... Is it just a refractoring that does not change the behavior of the code or is this pull request fixing any bugs? Please fill out the pull request template to clarify

Hello @liquidcronos,
sorry for not adressing the changes correctly. I updated the description now.

@MaHajo MaHajo closed this Mar 18, 2024
@MaHajo MaHajo reopened this Mar 18, 2024
@MaHajo
Copy link
Contributor

MaHajo commented Mar 18, 2024

Closing pull-request due to it beeing outdated.

@MaHajo MaHajo closed this Mar 18, 2024
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