Skip to content
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

Added normalize transform option to normalize string spaces #217

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

yusuf-khamis
Copy link

I added a normalize option for transform, a normalize option just basically removes multiple spaces and replaces them with a single space, I have been custom doing this in my previous projects and I thought this might be useful

Copy link
Member

@epoberezkin epoberezkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Added some comments

@@ -26,6 +27,7 @@ const transform: {[key in TransformName]: Transform} = {
toLowerCase: (s) => s.toLowerCase(),
toUpperCase: (s) => s.toUpperCase(),
toEnumCase: (s, cfg) => cfg?.hash[configKey(s)] || s,
normalize: (s) => s.replace(/\s\s+/g, " "),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just \s+ ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...or [\s\t]+ if tabs were to be included too

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because \s+ means single space or more, and in my case i want to only check multiple spaces and reduce them to one space

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, pretty much the regex \s\s+ catters for all spaces, tabs and line breaks, maybe i add more tests for these other scenarios, but will also improve the regex to /(\s\s+)|\t|\n/ for our own peace of mind

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performance of replacing /\s+/ can be better than /\s\s+/ and the result will be exactly the same.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check my latest commits, i have resolved these issues and also added trimInner option

README.md Outdated
@@ -220,6 +220,7 @@ A standalone string cannot be modified, i.e. `data = 'a'; ajv.validate(schema, d
- `toLowerCase`: convert to lower case
- `toUpperCase`: convert to upper case
- `toEnumCase`: change string case to be equal to one of `enum` values in the schema
- `normalize`: replace inner multiple spaces with a single space
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is too generic for what it does... Possible alternative - trimInner (and should it not replace tabs too?).

Any better ideas?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

word trim means to remove space, this just removes extra spaces, its not a bad idea though to also include remove spaces completely, with trimInner and come up with a better name for this one like normalizeSpaces

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's please not conflate two different things into on PR.

I don't know the use case for removing all inner spaces tbh, normalizeSpaces is ok though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will separate the other one in a different PR, will make another commit today

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review my latest commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants