MARP-637: Cells with "wrong" numbers should not be reconized as numbers

[linhpd-axonivy]

No description provided.

[github-actions]

Test Results

9 tests  +9   9 ✅ +9   9s ⏱️ +9s
6 suites +6   0 💤 ±0 
6 files   +6   0 ❌ ±0 

Results for commit c76ce8c. ± Comparison against base commit 3eabbec.

♻️ This comment has been updated with latest results.

[ntqdinh-axonivy]

LGTM

[linhpd-axonivy]

Hi @ivy-rew ,
I made some enhancements to this one regarding datatype recognition. Could you help me take a look at it?

[ivy-rew]

please cover the new functionality with tests;

[ivy-rew]

LGTM

No, I disagree ... that doesn't look good to me. @ntqdinh-axonivy
I'd really keep the ivy market a place where professional development culture can be seen by everyone.

there are several reasons why such changes should come with a test:

• this repository has tests, it's quality should be kept and not declined 📉
• we extended a simple utility class, it's perfectly testable with simple fast executing unit tests ✔️
• having tests would it make much easier for me as reviewer, to see what you are trying to change here 🤔
• it seems as if you fixed a bug, bugs always scream for being covered with a test, unless you want the same functionality to be broken again in the near future without being noticed -> big frustration for the customer 🐞 😠
• this is product development; not project work ... products stay much longer on the market and will be used by a wider user base, so acting professional becomes way more important. 🥇

[linhpd-axonivy]

LGTM

No, I disagree ... that doesn't look good to me. @ntqdinh-axonivy I'd really keep the ivy market a place where professional development culture can be seen by everyone.

there are several reasons why such changes should come with a test:

• this repository has tests, it's quality should be kept and not declined 📉
• we extended a simple utility class, it's perfectly testable with simple fast executing unit tests ✔️
• having tests would it make much easier for me as reviewer, to see what you are trying to change here 🤔
• it seems as if you fixed a bug, bugs always scream for being covered with a test, unless you want the same functionality to be broken again in the near future without being noticed -> big frustration for the customer 🐞 😠
• this is product development; not project work ... products stay much longer on the market and will be user by a wider user base, so acting professional becomes way more important. 🥇

Thank you for your feedback. I'll update the test soon

[linhpd-axonivy]

Hi @ivy-rew ,
I have adapted the test for my updated code.

[ivy-rew]

why is this not interpreted as numeric value, just like excel does? I think this is what the customer was asking for 🤔

[ivy-rew]

please check with @ivy-sgi if we really got the customers requirements right; I think numbers that excel treat as numbers, should be numeric in ivy as well.

[linhpd-axonivy]

From the linked issue, the customer wants '15024.00.00' to be treated as text, and I checked that in my Excel, '15024.00.00' is recognized as text by default. Numeric values are right-aligned, while text is left-aligned.

[ivy-rew]

yeah I see, the customers sample file doesn't match the expectations written in prose. And I agree with you on the left/right alignment. Please check the customers file and you will see exactly such numbers as in your example ... but not left aligned ... as these are numbers.

I think we should cover the customers example sheet in a anonymized way: in his example, he has such numbers which are recognized as being 'numeric' by excel. And IMHO these numeric values should remain numeric in ivy too. Otherwise that feature request may makes sense to one customer, but not to the rest of the world.
🤷

[linhpd-axonivy]

I updated the tests for 2 cases.

• Column contains texts that look like numbers but in incorrect format should be determined as String
• Column contains both text and numeric values should be determined as String

[ivy-rew]

thanks for the update; the intention is now very clear.
It seems like libre-office calc, made me think that these "incorrect numbers" are totally normal normal numeric values. However; if I try to use them in a function they do not work as numbers ... so I finally understood the problem and agree on the direction to go 😅

[linhpd-axonivy]

Thanks for your feedbacks