don't warn for identical units when resolving symbols from u"..."

[mweastwood]

This is just a quick patch I whipped up to try and address #94. Basically, if two units have the same hash, we will no longer warn about finding two copies of the same unit. We also check that these symbols are actually units before warning (I think just having a symbol named "s" was enough to get the warning).

Unfortunately there is a drawback to the approach I've used here. Because Dicts are unordered, I think there's no longer any guarantee about which unit is selected in the event that there are in fact multiple units with the same name.

Before:

julia> using Unitful

julia> module UnitfulTest
           import Unitful
           using Unitful: @unit, s
           @unit twoseconds "twoseconds" TwoSeconds 2s false
           Unitful.register(UnitfulTest)
       end
UnitfulTest

julia> uconvert(u"s", 1*u"twoseconds")
WARNING: Symbol s was found in multiple registered unit modules. We will use the one from UnitfulTest.
2//1 s

After:

julia> using Unitful

julia> module UnitfulTest
           import Unitful
           using Unitful: @unit, s
           @unit twoseconds "twoseconds" TwoSeconds 2s false
           Unitful.register(UnitfulTest)
       end
UnitfulTest

julia> uconvert(u"s", 1*u"twoseconds")
2//1 s

julia> module UnitfulTest2
           import Unitful
           using Unitful: @unit
           @unit s "weird" Weird 1Unitful.m false
           Unitful.register(UnitfulTest2)
       end
UnitfulTest2

julia> uconvert(u"s", 1*u"twoseconds")
WARNING: Symbol s was found in multiple registered unit modules. We will use the one from UnitfulTest.
2//1 s

[ajkeller34]

I updated your PR to address the issue you pointed out. Do you want to take a look and see if you agree with the changes? (btw, your example didn't quite work as advertised, the last warning should say UnitfulTest2 right?)

P.S. It occurred to me that currently macroexpand(:(u"m/s")) yields :(Unitful.m / Unitful.s), but in most cases we could simply return e.g. m/s, such that any computations inside are done at parse time. I played around with this and couldn't come up with any non-pathological cases where the change was problematic, but I got cold feet and didn't include the change in this commit.

[coveralls]

Coverage remained the same at 88.3% when pulling cb697fd on mweastwood:better-accounting into c266d71 on ajkeller34:master.

[mweastwood]

I think in my implementation, if there were multiple units, the one that is selected was essentially random and I wouldn't have been surprised to see it change after restarting Julia. I had a comment to that effect.

I realize I broke some of the tests you had written by returning m/s. I think that change should have been ok in principle, but I agree with your changes as well. I think there are several places in Unitful where it could be doing more work at parse time.

Anyway I'm happy with the changes you've made as well. Looks good to me!

[codecov-io]

Codecov Report

Merging #98 into master will not change coverage.
The diff coverage is 90.9%.

@@           Coverage Diff           @@
##           master      #98   +/-   ##
=======================================
  Coverage   88.29%   88.29%           
=======================================
  Files          10       10           
  Lines         641      641           
=======================================
  Hits          566      566           
  Misses         75       75

Impacted Files	Coverage Δ
src/user.jl	91.57% <90.9%> (ø)	⬆️

---

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 c266d71...cb697fd. Read the comment docs.

[ajkeller34]

I'll just make a separate commit that changes the macro behavior to return m/s so that we can revert easily if it turns out to be a bad idea somehow. Thanks!