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

Refactor CamelCaseToSeparator Filter #186

Merged
merged 4 commits into from
Nov 4, 2024

Conversation

ramchale
Copy link
Contributor

Q A
Documentation no
Bugfix no
BC Break yes
New Feature no
RFC no
QA no

Description

Refactoring CamelCaseToSeparator based on #177

src/Word/CamelCaseToSeparator.php Outdated Show resolved Hide resolved
src/Word/CamelCaseToSeparator.php Outdated Show resolved Hide resolved
src/Word/CamelCaseToSeparator.php Outdated Show resolved Hide resolved
src/Word/CamelCaseToDash.php Outdated Show resolved Hide resolved
src/Word/CamelCaseToUnderscore.php Outdated Show resolved Hide resolved
test/Word/CamelCaseToSeparatorTest.php Outdated Show resolved Hide resolved
docs/book/v3/migration/v2-to-v3.md Show resolved Hide resolved
@gsteel
Copy link
Member

gsteel commented Oct 24, 2024

@ramchale

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

@ramchale ramchale force-pushed the CamelCaseToSeparator branch from b79a399 to 266111f Compare October 24, 2024 09:46
@ramchale
Copy link
Contributor Author

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

@ramchale
Copy link
Contributor Author

@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 😄

src/Word/CamelCaseToDash.php Outdated Show resolved Hide resolved
src/Word/CamelCaseToUnderscore.php Outdated Show resolved Hide resolved
src/Word/CamelCaseToSeparator.php Outdated Show resolved Hide resolved
@gsteel
Copy link
Member

gsteel commented Oct 24, 2024

@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?

@gsteel gsteel added this to the 3.0.0 milestone Oct 24, 2024
@gsteel gsteel linked an issue Oct 24, 2024 that may be closed by this pull request
@ramchale
Copy link
Contributor Author

ramchale commented Oct 25, 2024

@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
Copy link
Member

gsteel commented Oct 25, 2024

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

@gsteel gsteel mentioned this pull request Oct 25, 2024
54 tasks
@ramchale
Copy link
Contributor Author

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

@gsteel gsteel requested a review from froschdesign October 31, 2024 20:41
@gsteel gsteel linked an issue Oct 31, 2024 that may be closed by this pull request
Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

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

Some changes to the documentation are required.

- `setSeparator`
- `getSeparator`

The filter will now treat numbers as a word boundary. For example `ThisHas4Words` will filter to `This-Has-4-Words`
Copy link
Member

Choose a reason for hiding this comment

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

This new behaviour is not covered in the documentation. Example:

TIP: **New Behaviour since Version 3**
The filter will now treat numbers as a word boundary.
For example `ThisHas4Words` will filter to `This-Has-4-Words`.

- `setSeparator`
- `getSeparator`

The filter will now treat numbers as a word boundary. For example `ThisHas4Words` with the default separator will filter to `This Has 4 Words`
Copy link
Member

Choose a reason for hiding this comment

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

This new behaviour is not covered in the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

The note on constructor options is missing:

The constructor now only accepts an associative array of [documented options](../word.md#camelcasetoseparator).

- `setSeparator`
- `getSeparator`

The filter will now treat numbers as a word boundary. For example `ThisHas4Words` will filter to `This_Has_4_Words`
Copy link
Member

Choose a reason for hiding this comment

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

This new behaviour is not covered in the documentation.

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @ramchale

I'll wait for @froschdesign to approve the docs changes - I think behaviour wise, what we have is fine and any weird results from users supplying input that is not camel cased is the users problem.

@ramchale
Copy link
Contributor Author

ramchale commented Nov 1, 2024

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

docs/book/v3/migration/v2-to-v3.md Outdated Show resolved Hide resolved
docs/book/v3/migration/v2-to-v3.md Outdated Show resolved Hide resolved
docs/book/v3/migration/v2-to-v3.md Outdated Show resolved Hide resolved
@froschdesign
Copy link
Member

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

@ramchale ramchale force-pushed the CamelCaseToSeparator branch from bb45d13 to 9a91b32 Compare November 4, 2024 10:02
@gsteel gsteel self-assigned this Nov 4, 2024
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Thanks again @ramchale 👍

@gsteel gsteel merged commit b57f096 into laminas:3.0.x Nov 4, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CamelCaseToDashFilter & numbers Improve handling of digits in CamelCaseToSeparator filter
3 participants