MadsKirkFoged / EngineeringUnits

Working with units made easy with automatic unit-check and converting between units
MIT License
41 stars 10 forks source link

Actually fixing unicode symbols #50

Closed mschmitz2 closed 5 months ago

mschmitz2 commented 5 months ago

continuation of #49 where I didn't catch the failing unit tests after pasting in the unicode characters with '' inside the strings. Also, the degree symbol was automatically pasted as ° instead of \u00b0 in one location.

mschmitz2 commented 5 months ago

The tests that are now failing in here (TemperatureTest) are definitely passing on my machine. Fore some reason there seems to be ° symbols that are not there on my machine.

I'm not sure why this is happening. I also didn't do any changes in this file in my previous commits... @MadsKirkFoged any ideas?

Passing local tests:

image

Debugging locally in first failing test: no degree symbol in T3 so test is passing:

image

Source code of this test for reference:

    [TestMethod]
    public void CelsiusDivideFahrenheit()
    {

        //Arrange
        Temperature T1 = new(100, TemperatureUnit.DegreeFahrenheit);
        Temperature T2 = new(4, TemperatureUnit.DegreeCelsius);

        //Act
        UnknownUnit T3 = T2 / T1;
        var K3 = T2.As(TemperatureUnit.SI) / T1.As(TemperatureUnit.SI);

        //Assert
        Assert.AreEqual(T3.ToString(), "0.8914");
        Assert.AreEqual(K3, 0.89136455411224458);
    }
MadsKirkFoged commented 5 months ago

image image image Looks like it is only working on your computer :-D Github, Azure and my computer is failing the tests

mschmitz2 commented 5 months ago

I don't understand what would have changed since yesterday... the tests' source file has not been updated for 3 weeks

image

So it seems like this is a bug where UnknownUnit wrongly gets the ° symbol from the division operation between two temperatures (except that on my machine it doesn't)...

MadsKirkFoged commented 5 months ago

This is a bug I haven't though about before..

When we have an Unknownunit and try to cast it to known unit using IntelligentCast there are units like Ratio and Degree that can be casted to both unit. That means that the order suddenly matters. We might have to think about a good fix for this.

Right now I have created a temp fix: image

mschmitz2 commented 5 months ago

I still don't get why it catches on AngleUnit and ApparentPowerUnit though.. it doesn't for me, which is why my tests are passing:

image

Any why has this changed without any changes to these files?

MadsKirkFoged commented 5 months ago

I think I got to the bottom of it.

It started when you sorted the list in: https://github.com/MadsKirkFoged/EngineeringUnits/pull/47/commits/6543891ce51b797eb6c8a04098b02bebe7efea2c Which was a good thing that should have worked! And that I also though would work! But from it I learned that some units are the same and therefore the order matter.

I have reworked the list a bit and grouped all the similar units and then sorted the units in each list. In the future it could be nice with a solution where the order didn't matter but that is yet to be discovered :)

mschmitz2 commented 5 months ago

Nice! Tricky one this... should we open a new issue to track... the issue?