Refactor converter runner
Needs ReviewPublic

Authored by alex on Wed, Feb 5, 10:56 AM.

Details

Reviewers
broulik
ngraham
sitter
Group Reviewers
Plasma
Summary

The logic from the converter runner has been refactored and moved to seperate files.
Additionally some foreach marcos have been refactored and the numberValue of the query gets only
calculated once, instead of being calculated for each matching unit.

Test Plan

The plugin should work as before and the fractional units in https://phabricator.kde.org/D22869 are still supported.

Diff Detail

Repository
R114 Plasma Addons
Branch
converter_runner_refactoring (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22295
Build 22313: arc lint + arc unit
alex created this revision.Wed, Feb 5, 10:56 AM
Restricted Application added a project: Plasma. · View Herald TranscriptWed, Feb 5, 10:56 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
alex requested review of this revision.Wed, Feb 5, 10:56 AM
alex added a comment.Wed, Feb 5, 11:16 AM

While I was refactoring this project I haven't changed the string parsing logic itself.

What do you think about changing the parsing process using a regex, for example to the regex from https://regex101.com/r/EUDzQi/7.

This can make the parsing process/adding new features much easier.

alex added a reviewer: Plasma.Wed, Feb 5, 11:16 AM
sitter added a subscriber: sitter.Wed, Feb 5, 12:10 PM

Please make a cpps for you headers and move the function implementations there.

+1 to regex

runners/converter/converter_utilities.h
23 ↗(On Diff #75039)

Perhaps I am missing something but it seems to me that all the functions in here really should be statics inside the runner.cpp, don't you think?

runners/converter/converterrunner.cpp
69

Is there a particular reason you hold it as a reference? (also applies to a bunch of QStrings and QLists below). We generally do not hold Qt types as references unless there is a reason to I think.

Oh and seeing as you largely split out existing code you should preserve Petri's copyright in all files.

alex added a comment.Wed, Feb 5, 1:40 PM

Thanks for the quick answer, I will adjust the code/copyright.

The reason I moved the functions to new files is that the runner file was really big and after splitting it up it was working well and the code was more readable/maintainable.
But I wanted to wait before making conceptual changes, until I get feedback because of the regex.

After implementing this most of these functions will be obsolete anyway :-) .

alex updated this revision to Diff 75064.Wed, Feb 5, 5:51 PM

Use Regex for query parsing

alex added a reviewer: sitter.Wed, Feb 5, 6:09 PM
alex marked an inline comment as done.

The regex has been implemented and the code is now ~100 lines shorter but provides the same functionality
and the few remaining utility functions have been moved to the runner class.
This should also fix the issues you mentioned :-) .
PS: The regex now includes the of the converter supported currency symbols.

Looks so much better already!

I'd really love a unit test for this though. plasma-workspace's servicesrunner should have an example one

runners/converter/converterrunner.cpp
19

Please move this after the includes. If any header in the future decides to want to use the name as well we're in for an awkward debugging session.

57

I am sure we can do better than hardcoding currency symbols. Perhaps iterate QLocales and get the currencySymbol for each?
(needs checking that performance doesn't degrade)

59

I'd really prefer named capture groups. Popping data out of the captures by index is fairly terrible to read and also easier to mess up in the future.

69

unnecessary ref.

74

Please use more descriptive names than u1, such as unit1.
In fact, isn't unit1 "inputUnit" and unit2 "outputUnit"? If so I'd name them thusly.

75

unnecessary ref.

77

unit1 and unit2 should probably be unitString1 and unitString2 so the actual units can be unit1 and unit2. same for value I suppose.

92

s/u/unit

93

s/v/value

122

newline missing it seems

128

in all return statements you can use list initialization

return { ok, output };

runners/converter/converterrunner.h
49

stringToDouble and getValidatedNumberValue seem to have conflicting ideas of api design. either both should use the pair return or both should use the ok pointer.

51

Please use multiple lines to declare multiple members, even when they have the same type. Same line declaration is unnecessarily increasing reading complexity.

alex updated this revision to Diff 75359.Mon, Feb 10, 1:02 PM
  • Start implementing requested changes
  • Remove unnecessary class declaration
alex marked 11 inline comments as done.EditedMon, Feb 10, 1:24 PM

Hello,
I have found no way to get the currency symbols, other that hard coded.
If you have a look at https://phabricator.kde.org/source/kunitconversion/browse/master/src/currency.cpp$67 you can see that the symbols are concatenated to the string.
Because of this I don't know how to get just the symbols.

I found currently implemented ones by executing:

qInfo() << inputCategory.allUnits();

and then manually copying the symbols.

And if you try to get the symbols using

for(const auto &s: inputCategory.allUnits()){
        qInfo() << inputCategory.unit(s).symbol();
    }

You get results like:
"PLN"
"GBP"
"JPY"
"EUR"

And more symbols (than the already hard coded ones) are currently not supported by the KUnitConversion library.