Connect command.py with common.py and create new methods

[giannissc]

I can confirm that the mark ready for review button is not available when you first create the PR so I am pinging you here @voneiden

[giannissc]

Remaining tasks for this are to:

• Create documentation for command.py
• Create tests for new methods

Should I include those on this PR or should they be on a separate one? @voneiden

EDIT: Converting PR to draft until resolved

[codecov[bot]]

Codecov Report

Merging #28 (01adf4b) into unstable (80c962c) will increase coverage by 1.23%. The diff coverage is 94.89%.

@@             Coverage Diff              @@
##           unstable      #28      +/-   ##
============================================
+ Coverage     73.12%   74.36%   +1.23%     
============================================
  Files            33       33              
  Lines          2255     2352      +97     
============================================
+ Hits           1649     1749     +100     
+ Misses          606      603       -3     

Impacted Files	Coverage Δ
src/cq_cam/routers.py	52.94% <ø> (ø)
src/cq_cam/tests/test_drill.py	100.00% <ø> (ø)
src/cq_cam/tests/test_op3d.py	100.00% <ø> (ø)
src/cq_cam/tests/test_profile.py	100.00% <ø> (ø)
src/cq_cam/command.py	77.88% <89.85%> (+6.11%)	:arrow_up:
src/cq_cam/fluent.py	75.20% <100.00%> (+0.20%)	:arrow_up:
src/cq_cam/operations/base_operation.py	78.33% <100.00%> (ø)
src/cq_cam/tests/test_gcode.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[voneiden]

Yes, please include tests in the PR, currently I've set sonarcloud to require a 65% coverage before merge is allowed. Including documentation here or in a separate PR is fine either way.

I've tried adding a CODEOWNERS file in b3e5e1f - if that works it should assign me as a reviewer automatically when a PR is opened (excluding drafts).

[giannissc]

I am thinking of changing the structure of Command slightly. One thing that I don't understand is why does to_gcode() return (str, Vector)?

The second is why must Command have an attribute end

[voneiden]

why does to_gcode() return (str, Vector)?

That might just be legacy baggage. See if you can get rid of it. Technically you should be able to use the end position of the previous command but..

The second is why must Command have an attribute end

Probably because previously all (?) commands have been motion commands. Might make sense to create a MotionCommand(Command) subclass that defines the end vector. However now if you make it possible to have commands without an end vector, you'll need to consider the first case. Possibly keep track of the last known end position here:

https://github.com/voneiden/cq-cam/blob/b3e5e1f3942aeadd211387f2829129367b4e9767/src/cq_cam/fluent.py#L39

The previous position is used to optimize the gcode.

G0 X1 Y1 Z1
G0 X1 Y2 Z1

the coordinates (and G value) that did not change and are not relevant can be skipped producing

G0 X1 Y1 Z1
Y2

While technically it is not necessary to do this, it can be a significant file size reduction.

ReferencePosition class seems to be obsolete, feel free to delete.

[giannissc]

Ok now I understand the reasoning! I have played a bit with removing but it makes thing unnecessarily complicated. I was also thinking of creating to abstract sub-classes from Command a MotionCommand (as you suggested) and a ConfigCommand

[giannissc]

Just added the test cases. I need to add the documentation for the module and I will mark it as ready for review

[giannissc]

Why is sonar cloud failing every time? Is there anything that I can do on my end to fix it?

[voneiden]

Why is sonar cloud failing every time? Is there anything that I can do on my end to fix it?

This is a limitation of SonarCloud and forks on Github ( https://portal.productboard.com/sonarsource/1-sonarcloud/c/50-sonarcloud-analyzes-external-pull-request ). Basically the PR checks are run in the context of your fork repository, so it does not have access to the required SONAR_TOKEN secret key that is configured in my repository.

If you're interested, you could try creating an account at SonarCloud and I'll try adding you to my personal organization ( https://sonarcloud.io/organizations/voneiden/ ). After you've created an account

1) Head over to https://sonarcloud.io/account/security and create a new token 2) Head over to https://github.com/giannissc/cq-cam/settings/secrets/actions and add the token as a repository secret with the name SONAR_TOKEN

[giannissc]

I will do sonar cloud soon to get rid of all the errors

[giannissc]

SONAR_TOKEN is set up. What do you need to add me to your organisation? @voneiden

[voneiden]

@giannissc bleh, didn't work, looks like the PR workflow has no access to secrets from your account either. I need to see if I can still try working around this somehow.

But I've added you to the sonarcloud org anyway.

[giannissc]

Maybe I need to make another commit to retrigger the CI pipeline?

[voneiden]

Nah it's not going to work. I think I will try using the automatic analysis feature of sonarcloud, it shouldn't have this restriction although code coverage will be lost (but suppose that's not a big deal since there's codecov)

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[giannissc]

Alrighty, a little bit of architectural tweaking requested still. Otherwise everything's looking good.

Once that's settled and the code is ready to be merged I'd still like to clean up the commit history a bit. Are you interested giving it a shot or would you prefer me to handle it? I'm not sure what your experience level is with git (for example rebasing), but I can certainly try giving you some pointers to get going.

I am happy to do the rebase and I would appreciate the pointers. I make the changes today

[voneiden]

You use vscode, right? Maybe this is a good primer https://blog.delpuppo.net/why-i-love-gitlens-in-my-vscode-interactive-rebase

I suggest you create a branch and try out rebasing there. That way it's easier to just throw away the branch if things go south. I have a local copy of this PR too anyway, so no worries.

What I'd like to see is that the commit history is tidied up a bit

1) Squash/fixup strongly related commits together unless keeping them separate makes sense. This means minor fix commits get squashed with their original feature commit. 2) No reverse merge commits (like Merge branch 'voneiden:unstable' into unstable) unless absolutely necessary.

To avoid 2 in the first place, during development you can use git rebase instead of git merge and/or git pull --rebase instead of git pull.

Feel free to ask and feel free to throw in the towel. I know git can be overwhelming.

[giannissc]

I am thinking that I won't be using kwargs for the gcode methods as I will be removing them on the next PR. I will write more of my thoughts into the discussion @voneiden