search

m1l / Everything-Metric-Firefox

Firefox Addon for converting USCS sites to Metric / SI
https://addons.mozilla.org/en-US/firefox/addon/everything-metric-converter/
32 stars 7 forks source link

Fix #6, #29, #31 and #32 #34

Closed qsantos closed 1 year ago

qsantos commented 1 year ago

Hello @m1l,

You might have noticed I have been opening a few issues lately. But I do not want to just open issues, I want to close them as well! If you merge this PR, I intend to keep working on fixing other open issues.

To make sure I am not breaking anything, I have added automated tests, working function by function (hence the large number of commits). I have taken your test suite and automated it to detect any regression in the future. If you intend to review the changes rather than just check the test page, it might make more sense to look at the commit history.

The fixes brought by this MR came from fixing small issues detected by TypeScript. It looks like it also fixed the first case in #8, but not the second.

Additionally, I have changed a rounding threshold from 1.6 % to 3 % in roundNicely, because it seemed inconsistent with the description. Do tell me if I missed something and if this should be reverted.

Note: the PR contains many commits, but it might be best not to squash them, since it might be useful if we notice a regression later and want to understand why it happened.

Cheers!

Fix #6, #29, #31 and #32

m1l commented 1 year ago

This is quite a few changes, so I will check this in the next days and get merge / back to you

qsantos commented 1 year ago

Of course, thanks for letting me know!

qsantos commented 1 year ago

I have noticed a regression on the unit-only parsing for cooking.nytimes.com. I did not notice it earlier since I do not have automated testing for this yet. I have pushed 3 commits on this PR to fix this and will be adding some testing to avoid a reoccurrence.

By the way, thanks for the extension, it's very convenient!

m1l commented 1 year ago

This is great! Thank you for improving it and I apologize for the messy code - I actually didn't think anyone would ever look at it, not to mention try to figure out what I was doing here. I went through all, and while I didn't notice any issues, I'll pack it and use it for a few days.

qsantos commented 1 year ago

Ah, and it's not #31 that was fixed by this PR, but number #33! Sorry about the mix-up! So I 'll let you check, but you should be able to close #29, #32 & #33!