Skip to content

Enhancement: Include $lower attribute for dashed#16

Merged
valorin merged 1 commit into
valorin:mainfrom
shatterproof:dashed-lower
Jan 11, 2024
Merged

Enhancement: Include $lower attribute for dashed#16
valorin merged 1 commit into
valorin:mainfrom
shatterproof:dashed-lower

Conversation

@shatterproof
Copy link
Copy Markdown
Contributor

By including $lower in the attributes for dashed it allows for the use cases of generating gift card codes or serial numbers (generally uppercase only) in a more efficient way than using strtoupper afterward.

@shatterproof
Copy link
Copy Markdown
Contributor Author

Looked into the random failed test and addressed here: #17

@valorin
Copy link
Copy Markdown
Owner

valorin commented Jan 11, 2024

Nice, I really like this!

Would an option to support upper be useful here as well? I guess you can apply upper on the output, but having it as part of it could be nice.

@valorin valorin merged commit dd47d28 into valorin:main Jan 11, 2024
@shatterproof
Copy link
Copy Markdown
Contributor Author

Currently when $lower is set to true for dashed (the default) both lowercase and uppercase will be used, when it is set to false only uppercase will be used. My goal here was to keep the attribute name consistent with other methods, but thinking about it again this might not be fully clear since one might think $lower as true means lowercase only.

What would you think about renaming this attribute only on dashed to $mixedCase instead of $lower for added clarity? Then when $mixedCase is true both uppercase and lowercase are used, when false only uppercase is used.

Alternatively if keeping the $lower naming consistent is desired, a comment could be added to this method's attribute to explain its behavior.

@shatterproof shatterproof deleted the dashed-lower branch January 11, 2024 18:47
@valorin
Copy link
Copy Markdown
Owner

valorin commented Jan 11, 2024

Ah good point, I misunderstood what it did, so the naming of $lower clearly doesn't work.

I like the idea of using $mixedCase instead - it's clearer what's going on.

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.

2 participants