Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Fix Quantity construction with value as double + unit as string #5235

Closed
wants to merge 1 commit into from

Conversation

0penBrain
Copy link
Contributor

https://forum.freecadweb.org/viewtopic.php?f=3&t=64261&start=20#p552178

This PR fixes construction of quantity with 'Quantity(double, QString)' that because of Unit implicit conversion leads to weird :
Quantity(1.0, QStringLiteral("m")) ==> "1 mm"

Now :

  • Unit(QString) is made explicit so no conversion can implicitly happen. Notice that behavior is unchanged so Unit(QStringLiteral("m")).getUnit() ==> "mm" but this looks correct to me as Unit has internally only unit dimensional awareness (no multiplier awareness)
  • Quantity has a dedicated Quantity(double, QString) constructor that tooks into account multiplier induced by unit so now Quantity(1.0, QStringLiteral("m")) ==> "1 m"

This PR has impact on #5232 so should be merged or rejected first.

I checked that currently nowhere in the master was used the problematic 'Quantity(double, QString)' construction and that's fine (no current use).

@freecadci
Copy link

pipeline status for feature branch PR_5235. Pipeline 425425698 was triggered at 2a4452f. All CI branches and pipelines.

@donovaly donovaly added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label Dec 9, 2021
@0penBrain 0penBrain changed the title [Core] Fix Quantity construction with value + unit as string [Core] Fix Quantity construction with value as double + unit as string Dec 9, 2021
@freecadci
Copy link

pipeline status for feature branch PR_5235. Pipeline 427881618 was triggered at 3f41c0b. All CI branches and pipelines.

Copy link
Contributor

@wwmayer wwmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catch(...) should be replaced with catch(const Base::Exception&)

@wwmayer
Copy link
Contributor

wwmayer commented Jan 21, 2022

Merged this PR as 2fef77e with the above mentioned change.
fb84f71 extends the Python wrapper and adds some unit tests

@wwmayer wwmayer closed this Jan 21, 2022
@0penBrain
Copy link
Contributor Author

catch(...) should be replaced with catch(const Base::Exception&)

I (stupidly) copied it from

catch (...) {

Should it be changed there also ?
Can you give some more explanation on this in order to improve next contributions ?

ANyway thx for merging, will now update #5232 accordingly and unleash it ;)

@wwmayer
Copy link
Contributor

wwmayer commented Jan 21, 2022

Should it be changed there also ?

Yes. Thanks for the hint. Searching for catch(...) in the current code base shows that it's used too often and this should be reworked where not appropriate.

Can you give some more explanation on this in order to improve next contributions ?

In many articles about coding guidelines it's considered bad practice to always handle exceptions with catch(...) because it may hide a deeper problem. The expression parser throws a Base::Exception (*) in case of a failure and only this should be handled. In case any other exception type is thrown the catch(...) swallows it and since it's not reported one would not even notice that there is a problem somewhere.

(*) IIRC the parser in the past has thrown Base::Exception but now it throws Base::ParserError. So, in the corresponding catch handles we should use Base::ParserError instead.

@0penBrain
Copy link
Contributor Author

@wmayer Thx for details

@0penBrain 0penBrain deleted the quantConstruct branch February 25, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants