Skip to content

Conversation

@maoalvin
Copy link

Here is a checklist you should tick through before submitting a pull request:

  • Implementation is clean
  • Code adheres to the existing coding standards; e.g. no curlies for one-line blocks, no redundant empty lines between methods or code blocks, spaces rather than tabs, etc.
  • No Code Analysis warnings
  • There is proper unit test coverage
  • If the code is copied from StackOverflow (or a blog or OSS) full disclosure is included. That includes required license files and/or file headers explaining where the code came from with proper attribution
  • There are very few or no comments (because comments shouldn't be needed if you write clean code)
  • Xml documentation is added/updated for the addition/change
  • Your PR is (re)based on top of the latest commits from the main branch (more info below)
  • Link to the issue(s) you're fixing from your PR description. fixes [Bug] Camelize() does not respect acronyms #1301 and fixes Let us configure Humanizer to treat acronyms as words #1534
  • Readme is updated if you change an existing feature or add a new one
  • Run either build.cmd or build.ps1 and ensure there are no test failures

@maoalvin maoalvin changed the title Pascalize with acronym Pascalize and Camelize with acronym Jun 25, 2025
@clairernovotny clairernovotny requested a review from Copilot June 26, 2025 11:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the inflector extension methods by adding optional acronym preservation to Pascalize and introducing a new CamelizeV2 for Google-style camel casing that properly handles acronyms.

  • Adds a preserveAcronym parameter and two private implementations (WithAcronymPreservation, WithoutAcronymPreservation) for Pascalize
  • Introduces CamelizeV2 leveraging the new Pascalize behavior
  • Updates unit tests, public API approvals, and README documentation

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

File Description
src/Humanizer/InflectorExtensions.cs Added preserveAcronym parameter to Pascalize, new internal methods, and CamelizeV2
src/Humanizer.Tests/InflectorTests.cs Added tests for acronym-preserving/non-preserving Pascalize and for CamelizeV2
src/Humanizer.Tests/ApiApprover/*.verified.txt Updated public API approval files to include new overloads and CamelizeV2
readme.md Documented the acronym option in Pascalize and introduced a CamelizeV2 section
Comments suppressed due to low confidence (3)

readme.md:654

  • The README description refers to Camelize but this is in the CamelizeV2 section; update the reference to CamelizeV2 for clarity.
`Camelize` camelizes acronyms correctly, it is backwards incompatible with the way current Camelize works which is why introducing a new version

src/Humanizer/InflectorExtensions.cs:130

  • [nitpick] Consider renaming CamelizeV2 to a more descriptive name like CamelizeWithAcronymPreservation to avoid versioned API names and improve readability.
    public static string CamelizeV2(this string input)

src/Humanizer.Tests/InflectorTests.cs:115

  • Test name PascalizeWithAcronymPreserved doesn’t match the method name PascalizeWithAcronymPreservation; consider aligning the naming for consistency.
    public void PascalizeWithAcronymPreserved(string input, string expectedOutput) =>


/// <summary>
/// By default, pascalize converts strings to UpperCamelCase also removing underscores
/// For a word that are all upper case, this function by default assumes its an acronym and preserves the casing
Copy link

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

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

[nitpick] The XML doc comment has a grammar issue: change 'For a word that are all upper case' to 'For words that are all uppercase' or 'For a word that is all uppercase'.

Suggested change
/// For a word that are all upper case, this function by default assumes its an acronym and preserves the casing
/// For a word that is all uppercase, this function by default assumes it's an acronym and preserves the casing

Copilot uses AI. Check for mistakes.
}
}

while (index < span.Length && (span[index] == ' ' || span[index] == '_' || span[index] == '-'))
Copy link

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

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

[nitpick] Repeated checks for the same separator characters could be extracted into a helper method or constant to improve maintainability and reduce duplication.

Suggested change
while (index < span.Length && (span[index] == ' ' || span[index] == '_' || span[index] == '-'))
while (index < span.Length && IsSeparator(span[index]))

Copilot uses AI. Check for mistakes.
@clairernovotny
Copy link
Member

Thank you for the contribution!

Couple thoughts:

  1. Might be better to call it "preserve upper-case" rather than acronyms as that appears to be the actual behavior?
  2. I think an overload to Camelize, with an enum parameter specifying behavior, is better than adding a CamelizeV2 as that's not particularly self-descriptive.

@maoalvin
Copy link
Author

maoalvin commented Jul 2, 2025

Thanks for the review !

Might be better to call it "preserve upper-case" rather than acronyms as that appears to be the actual behavior?

Sounds good I can make that change

I think an overload to Camelize, with an enum parameter specifying behavior, is better than adding a CamelizeV2 as that's not particularly self-descriptive.

I initially thought of that but could not come up with an appropriate enum. The current way this function camel cases acronyms seems like a bug in the sense it doesn't adhere to any camel case standard that I could find which is why I didn't want to overload that further with an additional parameter. In order to think of an appropriate enum there needs to be a way to describe the current form.

My preference would be to mark the current one as 'deprecated' to make it clear to existing clients that there is a more correct version available that they can migrate to. I can change CamelizeV2 to something like ToCamelCase with proper documentation stating that this is the correct format ? If we really want to stick to the enum approach then probably could use something like CamelCase.Legacy for current default version, but my personal preference is a new function

@clairernovotny
Copy link
Member

I like ToCamelCase as it's descriptive and fits the general naming conventions. As this will be for 3.0, the existing one can be marked as deprecated with a pointer to the new method. Please ensure docs are updated to reflect that as well and then I can merge the PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Let us configure Humanizer to treat acronyms as words [Bug] Camelize() does not respect acronyms

2 participants