kossebau added a comment.
Some quick feedback, though no in-detail review myself for now, not sure I will be able later, so needs others as well. INLINE COMMENTS > kformat.h:117 > + * @see formatValue > + */ > + enum class Unit { Please add missing "@since 5.48" (as last item, following https://community.kde.org/Frameworks/Frameworks_Documentation_Policy#Document_Public_and_Protected_Members) > kformat.h:134 > + * @see formatValue > + */ > + enum class UnitPrefix { @since 5.48 > kformat.h:339 > + * Example: > + * format.formatValue(1000, KFormat::Unit::Bit, 1, > KFormat::UnitPrefix::Kilo) => "1.0 kbit" > + * Please wrap example code with tags for nice highlighting, and for that adapt also string to be real code, e.g. like this: * @code * // sets value to "1.0 kbit" * auto value = format.formatValue(1000, KFormat::Unit::Bit, 1, KFormat::UnitPrefix::Kilo); * @endcode > kformat.h:356 > + * @see BinaryUnitDialect > + */ > + QString formatValue(double value, @since 5.48 > kformat.h:367-369 > + * format.formatValue(1000, QStringLiteral("bit"), 1, > KFormat::UnitPrefix::Kilo) => "1.0 kbit" > + * format.formatValue(1000, QStringLiteral("bit/s") => "1.0 kbit/s" > + * format.formatValue(12.3e6, QStringLiteral("bit/s") => "12.3 Mbit/s" same code/endcode treatment please > kformat.h:380 > + * @see UnitPrefix > + */ > + QString formatValue(double value, @since 5.48 > kformat.h:382 > + QString formatValue(double value, > + QString unit, > + int precision = 1, pass by const reference -> const QString& unit That the unitstring argument is modified internally in the private method is a current implementation detail which should not leak into the public API > kformatprivate.cpp:138-145 > + case KFormat::Unit::Bit: > + unitString = QStringLiteral("bit"); > + break; > + case KFormat::Unit::Byte: > + unitString = QStringLiteral("B"); > + break; > + case KFormat::Unit::Meter: Are we sure those unit symbols do not need to be localized? What about languages using different scripts (cyrillic, chinese, arabic, etc)? > kformatprivate.cpp:154 > + //: value without prefix, format "<val> <unit>" > + return tr("%1 %2", "no Prefix").arg(numString).arg(unitString); > + } Please use multi-argument arg method with string arguments: arg(numString).arg(unitString) -> arg(prefixString, unitString) > kformatprivate.cpp:172 > + //: value with prefix, format "<val> <prefix><unit>" > + return tr("%1 %2%3", > "MetricBinaryDialect").arg(numString).arg(prefixString).arg(unitString); > +} arg(prefixString, unitString) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D13583 To: bruns, #frameworks Cc: kossebau, kde-frameworks-devel, astippich, michaelh, ngraham, bruns