Skip to content

Fix issue tools::toTitleCase() does not accept factos as input#92

Merged
elong0527 merged 4 commits into
mainfrom
91-the-variable-in-the-ae-listing-does-not-accept-factor
Aug 19, 2025
Merged

Fix issue tools::toTitleCase() does not accept factos as input#92
elong0527 merged 4 commits into
mainfrom
91-the-variable-in-the-ae-listing-does-not-accept-factor

Conversation

@wangchen46
Copy link
Copy Markdown
Collaborator

Replace tools::toTitleCase() to function propercase.

@wangchen46 wangchen46 linked an issue Aug 15, 2025 that may be closed by this pull request
@fukuhiro2023
Copy link
Copy Markdown
Collaborator

Thank you for your update. I believe the functionality of propercase() in forestly is slightly differenet from that of tools::toTitleCase().

tools::toTitleCase("american indian or alaska native") 
# > "American Indian or Alaska Native"

forestly:::propercase("american indian or alaska native")
# > "American indian or alaska native"

It may be better to keep tools::toTitleCase(), but add as.character() before it to ensure successful conversion when the input is a factor.

… RACE if factor or character vectror. If factor, convert to character vector as input of tools::toTitleCase().
@wangchen46
Copy link
Copy Markdown
Collaborator Author

@fukuhiro2023, Thank you! I revert to use tools::toTitleCase(). I have added check for SEX if input is factor or not. If factor, will convert to character vector before putting into tools::toTitleCase(). For other inputs (i.e RACE, EPOCH), type of these inputs has converted to be character vector when applying tolower::() as tested, and thus I did not add check for these inputs but only SEX. Let me know your thoughts.

@elong0527
Copy link
Copy Markdown
Collaborator

elong0527 commented Aug 18, 2025

I would suggest to write a helper function for pre-processing of the input data.

The current implementation will break the order defined in factor that will introduce unexpected behavior from user perspective. Specifically:

  • based on input, identify all variables required.
  • if a variable is a char using tools::toTitleCase()
  • if a variable is a factor change the levels using tools::toTitleCase()
x <- factor(c("there is one", "that is two", "here is three"))
levels(x) <- tools::toTitleCase(levels(x))
x

Chen Wang and others added 2 commits August 18, 2025 14:07
- Enhanced propercase() and titlecase() to handle factors
- Replaced direct toTitleCase calls with titlecase() wrapper
- Added unit tests for both functions

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@elong0527
Copy link
Copy Markdown
Collaborator

I just updated propercase and titlecase to handle both factor and char with AI.

unit test are added.

Feel this is sufficient for this PR to close #92.

However, we should consider if we really need format_ae_listing. Maybe a wrapper of prepare_ae_listing would be sufficient as its intention is to generate a AE listing.

https://merck.github.io/metalite.ae/reference/prepare_ae_listing.html

@elong0527 elong0527 self-requested a review August 18, 2025 23:37
@elong0527 elong0527 merged commit d7d6e97 into main Aug 19, 2025
7 checks passed
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.

The variable in the AE listing does not accept factor

3 participants