Shortcut conversion factor

[skeating]

No description provided.

[codecov]

Codecov Report

Merging #98 into develop will increase coverage by 0.06%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop      #98      +/-   ##
===========================================
+ Coverage    93.54%   93.61%   +0.06%     
===========================================
  Files            7        7              
  Lines          914      924      +10     
  Branches       198      200       +2     
===========================================
+ Hits           855      865      +10     
  Misses          34       34              
  Partials        25       25

Impacted Files	Coverage Δ
cellmlmanip/units.py	95.87% <100%> (+0.14%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5284857...371ed78. Read the comment docs.

[MauriceHendrix]

This is fine in general, but it does have the danger of becoming a bit confusing.
I would say update the docstring to say that it requires a to_unit and either a to_unit, a quantity or an expression.

At the start of the method there are some useful asserts. It would be useful to assert that only 1 of from_unit, quantity, expression should be defined.
You could build a big if statement or do something like
assert [from_unit, quantity, expression].count(None) == 2

Also might it be worth in this case giving the parameters an explicit type (see for example add_equation), I think this would reduce potential for confusion.

[MauriceHendrix]

Just the consistency between the description and the assert (see comment above)

[jonc125]

The approach looks fine but I spotted a couple of minor issues. Also the travis tests are failing; I think due to flake8 errors.

[MichaelClerx]

Hey @MauriceHendrix , do you approve of the changes?

[MauriceHendrix]

Hey @MauriceHendrix , do you approve of the changes?

looks good to me. I'll need to update some code on my end but shouldn't be a big deal. Has it been marged into the master yet?

[skeating]

@MauriceHendrix You need to confirm the changes/dismiss the review when you asked for changes before I can merge

[MauriceHendrix]

@MauriceHendrix You need to confirm the changes/dismiss the review when you asked for changes before I can merge

Aah ok I didn't realise