search

elmcraft / core-extra

Utility functions for an improved experience with elm/core
https://package.elm-lang.org/packages/elmcraft/core-extra/latest/
Other
22 stars 10 forks source link

Different Results for `dasherize` and `clean` #48

Closed ErikFeeley closed 2 months ago

ErikFeeley commented 4 months ago

Describe the bug dasherize has different outputs when compared across the old String.Extra versus the current implementation from here. clean also processes strings where words have a \n between them without spaces differently.

To Reproduce Steps to reproduce the behavior: Please check out this Ellie. I copied only what I needed from the old and new implementations to examine.

Expected behavior Ideally upgrading from string-extra 4.0.1 to core-extra 1.0 would not change existing behavior.

Additional context When attempting the upgrade from various *-extra libraries to core-extra 1.0 I discovered these issues via our tests. I see that this was discussed over here and the history goes back all the way to 2017. Well, related to the leading dash that is. I haven't found any additional context around clean though. Just kidding found this :).

Lastly I want to say thanks again for putting core-exra together. And if I can find time I would like to contribute to the project!

gampleman commented 4 months ago

Hi @ErikFeeley. Some of these changes were made (as you found out) to make String.Extra’s own tests pass. I thought that the new versions were fixing bugs in the old ones (and we didn’t promise bug-to-bug compatibility).

So what I think we’d need for this issue is some argument on why the old behaviour is better or more predictable. Which usecases does the old version work for where the new one doesn’t?

ErikFeeley commented 4 months ago

Hey @gampleman thanks for the update!

So I can't in good faith make an argument for why the old behavior is better / more predictable. All I know is that during my upgrade attempt at work I noticed some of our tests failing on functionality that was built on top of dasherize and/or clean.

I missed the bug-to-bug compatibility thing I guess, so thats on me! If at the end of the day the answer is that I have to change our implementations a bit for our test to pass then all good! It will just take a bit longer to finish the upgrade :).

The only other thing I could think of would be to provide additional old implementations of those functions somehow, which would allow for a 1 to 1 upgrade but then the same deprecation mechanism could be used for moving to V2 maybe? Based on my single case so far though I am not convinced that effort would be worth it.

gampleman commented 4 months ago

Perhaps then the action here to take is to clarify this in the readme. I think it's fair to say that the upgrade instructions do give the impression of bug-to-bug compatibility, even though it would be very annoying and difficult for us to guarantee it.

ErikFeeley commented 4 months ago

@gampleman That sounds good to me!