Details

Reviewers

patrickelectric
laysrodrigues
tcanabrava

Commits

R232:8167a016fa60: Clean up gcodecommands. update tests and use in atcore and testclient
R232:529dd8130a4c: Improve GCodeCommands

Summary

From the old Diff

Use Teareny opperation to make code more readable. Simplify Error Message

New in this Diff

Rename GCodeCommands::toString -> GCodeCommands::description.
Improve Error Message for when a Gcode is needed.

Diff Detail

Repository

R232 AtCore

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

rizzitello requested review of this revision.Feb 11 2018, 2:46 AM

rizzitello created this revision.

rizzitello edited the summary of this revision. (Show Details)Feb 11 2018, 2:49 AM

rizzitello added reviewers: patrickelectric, laysrodrigues, tcanabrava.

tcanabrava accepted this revision.Feb 11 2018, 12:03 PM

This revision is now accepted and ready to land.Feb 11 2018, 12:03 PM

Why we are doing tests with descriptions ?

In D10436#204418, @patrickelectric wrote:

Why we are doing tests with descriptions ?

For exactly the same reason we were checking the toString calls to have full test coverage.

rizzitello mentioned this in D10299: Begin support for Sd cards.Feb 11 2018, 7:44 PM

In D10436#204545, @rizzitello wrote:

In D10436#204418, @patrickelectric wrote:

Why we are doing tests with descriptions ?

For exactly the same reason we were checking the toString calls to have full test coverage.

@tcanabrava make sense to do a test of hardcoded text string ?

src/gcodecommands.cpp
94	QString code("G" + QString::number(gcode));

Remove tests for GCodeCommands::description
Use common code for switches when possible

rebase to master

patrickelectric requested changes to this revision.Feb 12 2018, 2:54 PM

patrickelectric added inline comments.

src/gcodecommands.cpp
96–97	Move to default.
107	Return gcode by default, check gcode range for error.
502–503	Move to default.

This revision now requires changes to proceed.Feb 12 2018, 2:54 PM

rizzitello added inline comments.Feb 12 2018, 2:58 PM

src/gcodecommands.cpp
107	We don't want to return any gcode we have not added support for . This does not prevent anyone from using the code but does stop them from getting any code via GCode functions . There is no good default either some code will work without options and some wont.

patrickelectric added inline comments.Feb 13 2018, 11:30 AM

src/gcodecommands.h
141	We can change this to protect and and use it inside GCodeTest.
unittests/gcodetests.cpp
79	I see the same test around, maybe a simple function to check if argument is necessary. QVERIFY(needArgument(GCode::M104))

protect the error string GCodeCommands
use a function for the needs argument tests

patrickelectric added inline comments.Feb 13 2018, 2:07 PM

src/gcodecommands.cpp
106	This can be a private class string.
106	protected*
unittests/gcodetests.cpp
71	You can inherit GCode class into GCodeTects, than you'll be able to use the protected strings for your tests.

Gcode and GCodeTest are now friends

patrickelectric requested changes to this revision.Feb 13 2018, 3:04 PM

patrickelectric added inline comments.

unittests/gcodetests.cpp
25	return condition
71	return Code::toCommand(code) == GCode::commandRequiresArgument.arg(QStringLiteral("M"), QString::number(code));

This revision now requires changes to proceed.Feb 13 2018, 3:04 PM

clean up return for testGCodeNeedsArg testMCodeNeedsArg

Great job ! LGTM !

This revision is now accepted and ready to land.Feb 13 2018, 3:13 PM

Update GcodeCommands

rizzitello closed this revision.Feb 13 2018, 9:28 PM

Closed by commit R232:8167a016fa60: Clean up gcodecommands. update tests and use in atcore and testclient (authored by rizzitello). · Explain WhyFeb 13 2018, 9:28 PM

This revision was automatically updated to reflect the committed changes.

			Path	Packages
M			src/atcore.cpp (2 lines)
M			src/gcodecommands.h (110 lines)
M			src/gcodecommands.cpp (137 lines)
M			testclient/mainwindow.cpp (12 lines)
M			unittests/gcodetests.h (216 lines)
M			unittests/gcodetests.cpp (1081 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	26901	5697d24		Feb 11 2018, 2:46 AM	★	★
Diff 2	26973	5697d24	- Remove tests for GCodeCommands::description	Feb 12 2018, 1:56 AM	★	★
Diff 3	26974	c2caa1e	- rebase to master	Feb 12 2018, 1:59 AM	★	★
Diff 4	27060			Feb 13 2018, 2:01 PM	★	★
Diff 5	27068			Feb 13 2018, 2:58 PM	★	★
Diff 6	27069			Feb 13 2018, 3:09 PM	★	★
Diff 7	27108	c2caa1e	- Update GcodeCommands	Feb 13 2018, 9:28 PM	★	★
Diff 8	27109	c2caa1e	R232:8167a016fa60637d6500cbbed420a8270526e6de	Feb 13 2018, 9:26 PM	★	★

Improve GCodeCommands
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 27109

src/atcore.cpp

src/gcodecommands.h

src/gcodecommands.cpp

testclient/mainwindow.cpp

unittests/gcodetests.h

unittests/gcodetests.cpp

Improve GCodeCommandsClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 27109

src/atcore.cpp

src/gcodecommands.h

src/gcodecommands.cpp

testclient/mainwindow.cpp

unittests/gcodetests.h

unittests/gcodetests.cpp

Improve GCodeCommands
ClosedPublic
Actions

Revision Contents
Changeset List