laminas / laminas-filter

Programmatically filter and normalize data and files
https://docs.laminas.dev/laminas-filter/
BSD 3-Clause "New" or "Revised" License
85 stars 35 forks source link

Refactor CamelCaseToSeparator Filter #186

Closed ramchale closed 3 weeks ago

ramchale commented 1 month ago
Q A
Documentation no
Bugfix no
BC Break yes
New Feature no
RFC no
QA no

Description

Refactoring CamelCaseToSeparator based on https://github.com/laminas/laminas-filter/issues/177

gsteel commented 1 month ago

@ramchale

Had another look at the regex - I think this is more like it: https://3v4l.org/ISIYe

ramchale commented 1 month ago

Thanks I think the solution to #10 needs some discussion, so I've split that into it's own Draft PR

ramchale commented 1 month ago

@ramchale

Had another look at the regex - I think this is more like it: https://3v4l.org/ISIYe

Awesome. Hadn't seen this before I created the additional draft. I'd put in my own unicode example, but your change seems to work for that as well 😄

gsteel commented 1 month ago

@froschdesign Can I have your opinion on the use of str_replace here?

https://github.com/laminas/laminas-filter/pull/186#discussion_r1814793452

I can't decide if input like 'Foo-Bar' should be filtered to 'Foo-Bar' or 'Foo---Bar' - I'm currently preferring the latter because it is consistent with 'Foo/Bar' => 'Foo-/-Bar'.

WDYT?

ramchale commented 1 month ago

@froschdesign Can I have your opinion on the use of str_replace here?

#186 (comment)

I can't decide if input like 'Foo-Bar' should be filtered to 'Foo-Bar' or 'Foo---Bar' - I'm currently preferring the latter because it is consistent with 'Foo/Bar' => 'Foo-/-Bar'.

WDYT?

A third option is to add an entry to the constructor Options (something like $this->deduplicateSeperator = $options['deduplicateSeperator'] ?? false;) to toggle the use of the str_replace. Would also need to add this to CamelCaseToUnderscore and CamelCaseToDash as it currently stands.

gsteel commented 1 month ago

A third option is to add an entry to the constructor Options (something like $this->deduplicateSeperator = $options['deduplicateSeperator'] ?? false;) to toggle the use of the str_replace. Would also need to add this to CamelCaseToUnderscore and CamelCaseToDash as it currently stands.

If we're doing extra options - it would be better to perhaps to add stripNonWord and then preg_replace anything else with a single separator, so:

I'mAMuppet becomes I-m-A-Muppet, but at this point, we're trying to save devs from themselves - this filter is supposed to convert camelcase input, if you've not provided camelCase input then any weird output is on you, so I'm in favour of filtering With-Dashes to With---Dashes and removing any attempt to normalise input.

Also, remember that filter chain can be used, i.e. Trim | Alnum | CamelCaseToDash

ramchale commented 1 month ago

Updated this in line with the conversation so far. Let me know if it needs further changes

ramchale commented 3 weeks ago

@gsteel awesome, thanks. I think you solved the trickiest bit 😄

froschdesign commented 3 weeks ago

@gsteel Before the release I will check and update the documentation again.