sodiray / radash

Functional utility library - modern, simple, typed, powerful
https://radash-docs.vercel.app
MIT License
4.31k stars 173 forks source link

`snake` not working same as lodash's `snakeCase` #248

Closed apstanisic closed 1 year ago

apstanisic commented 1 year ago

It does not add _ between char and number codesandbox

import { snakeCase } from "lodash";
import { snake } from "radash";

const valWithNumber = "helloWorld23Test";

const lod = snakeCase(valWithNumber);
// hello_world_23_test
const rad = snake(valWithNumber);
// hello_world23_test
sodiray commented 1 year ago

Interesting, thanks for bringing this up @apstanisic 👍 We can fix this, it would be easy, and I'm certainly willing but I'm not sure we should at this point, but I'd love user's input.

My hesitation is that changing would require a major bump and even if we do a major bump this is one of those sneaky changes that could easily cause bugs during an upgrade for users. On top of that, lodash parody is not necessarily a goal.

For example I use dash, a sister function to snake, in some web scraping systems to genereate uids from scraped content. If the dash function unexpectadly produced new results it would really cause issues. I could be a unique use case though 🤷

What do you think?

apstanisic commented 1 year ago

This is kinda like python 2 -> 3 situation, it can break apps without knowing. If users switch from lodash, they might think that this function does the same thing, and can cause runtime problems.

If this change is made, it should wait for next major bump. Easiest upgrade path would be to change default behavior to match lodash's version in next major version, and to add option object to snake function. That way user can freely switch from lodash, and current radash users will read about breaking change in docs and can decide how to proceed.

type SnakeFn = (val: string, options?: { splitOnNumber?: boolean }) => string

Or just add warning in the docs that this function does not work the same as lodash's version

Personally I find i_have_5_apples more readable than i_have5_apples, but this is subjective.

sodiray commented 1 year ago

I agree with your subjective take, i_have_5_apples is way better. This example is a bit eye-opening, regardless of what Lodash is doing this is the right result.

Let's make the change, PR it, and hold it for the next major release 🚀

You can make the change/PR if you like, if not I can. No big rush on this one.

apstanisic commented 1 year ago

I'll look into it. This is dirty fix that I'm currently using:

function snakeCase(val: string): string {
    return snake(val).replace(/([A-Za-z]{1}[0-9]{1})/, (val) => `${val[0]!}_${val[1]!}`)
}