-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Different Results for dasherize
and clean
#48
Comments
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? |
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 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. |
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. |
@gampleman That sounds good to me! |
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 tocore-extra
1.0 would not change existing behavior.Additional context
When attempting the upgrade from various
*-extra
libraries tocore-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 aroundclean
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!The text was updated successfully, but these errors were encountered: