To update this diff without accidentally creating a new one, make sure you're on the permeability branch in your kunitconversion repo, then make your change and run arc diff. It shouldn't try to create a new one.

src/unit.h
336	Can you add `/** @since 5.53 */` above each new one, following the pattern?
src/volume.cpp
452	I'm not sure this should be marked as a common unit. Because it is, I now get oil barrels listed first when I have KRunner convert something like "2 cups" or "8 liters".

This revision now requires changes to proceed.Nov 5 2018, 1:53 PM

ngraham added inline comments.Nov 5 2018, 2:33 PM

src/permeability.cpp
4	Since you created this file, this should be your own copyright.
src/permeability.h
3 ↗	(On Diff #44906)	Since you created this file, this should be your own copyright.

ngraham added inline comments.Nov 5 2018, 3:33 PM

src/unit.h
65	Please add `/** @since 5.53 */` here, following the pattern.

Reviewed with comments made on this diff.

Harbormaster completed remote builds in B4596: Diff 44914.Nov 5 2018, 3:52 PM

The header file for class Permeability should probably be named permeability_p.h to match the other category classes since it's a not exported implementation detail

src/unit.h
335	Given this is a new category, assign `Darcy = 33000` so that one can still add new values after `Yoctoohm` to electrical resistance category without breaking compatibility

permeability.h renamed to permeability_p.h and change Darcy to start at 33000

Harbormaster completed remote builds in B4597: Diff 44917.Nov 5 2018, 4:15 PM

joaonetto updated this revision to Diff 44918.Nov 5 2018, 4:16 PM

Harbormaster completed remote builds in B4598: Diff 44918.Nov 5 2018, 4:16 PM

Looking good now, +1

Looks great and works great! Nice first patch. Just one more thing is required before I can land it: add the following to the Summary section of the patch:

BUG: 388074
FIXED-IN: 5.53

Thanks!

This revision is now accepted and ready to land.Nov 6 2018, 12:14 AM

Done, can land now

Indeed it can. Great job! May it be the first of many. :-)

Closed by commit R292:b2419e57eeea: Updated with petroleum industry units (authored by joaonetto, committed by ngraham). · Explain WhyNov 6 2018, 9:11 PM

This revision was automatically updated to reflect the committed changes.

		Path
M		README.md (1 line)
M		src/CMakeLists.txt (1 line)
M		src/converter.cpp (2 lines)
A	M	src/permeability.cpp (63 lines)
A	M	src/permeability_p.h (36 lines)
M		src/unit.h (15 lines)
M		src/volume.cpp (10 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	44903	74c5370		Nov 5 2018, 1:21 PM	★	★
Diff 2	44904	74c5370	- Updated source code with permeability class	Nov 5 2018, 1:35 PM	★	★
Diff 3	44905	74c5370	Updated with permeability source code	Nov 5 2018, 1:38 PM	★	★
Diff 4	44906	74c5370	Couldn't update D16681, creating new revision	Nov 5 2018, 1:41 PM	★	★
Diff 5	44914	74c5370		Nov 5 2018, 3:51 PM	★	★
Diff 6	44917	74c5370		Nov 5 2018, 4:13 PM	★	★
Diff 7	44918	74c5370		Nov 5 2018, 4:16 PM	★	★
Diff 8	44991	74c5370	R292:b2419e57eeea5cd98b3bfb42c3032b421633bf39	Nov 6 2018, 9:11 PM	★	★

Updated with petroleum industry units
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 44991

README.md

src/CMakeLists.txt

src/converter.cpp

src/permeability.cpp

src/permeability_p.h

src/unit.h

src/volume.cpp

Updated with petroleum industry unitsClosedPublicActions