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

add std.uni.toCapitalized() #3167

Merged
merged 1 commit into from
Apr 18, 2015
Merged

Conversation

WalterBright
Copy link
Member

// Accept range of dchar's
if (isInputRange!Range &&
isSomeChar!(ElementEncodingType!Range) &&
ElementEncodingType!Range.sizeof == dchar.sizeof)
Copy link
Member

Choose a reason for hiding this comment

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

this looks redundant. first you check if it is some char than you check if the size equals dchar. why not is(ElementEncodingType!Range == dchar)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done this way at Andrei's suggestion because it needs to allow enums whose base type is dchar, but not char or wchar which are implicitly convertible to dchar.

Note that the bulk of this code is copy/pasta from toCase.

Copy link
Member

Choose a reason for hiding this comment

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

an string of enums?

if it copy/paste, shouldn't they be joined into a common base?

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't figure out a clean way to do it. If you can, a followon PR would be great.

Copy link
Member

Choose a reason for hiding this comment

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

I will take a look, only currently I'm starting to work on an std.xml replacement and before that I want #2995 to get merged.

@MetaLang
Copy link
Member

MetaLang commented Apr 9, 2015

toCapitalize is a poor name, but I can't think of anything much better than toCapital. Couldn't this be added as an overload of capitalize?

@WalterBright
Copy link
Member Author

It won't work as an overload of capitalize(), because both accept strings.

@WalterBright WalterBright force-pushed the toCapitalize branch 3 times, most recently from e8b2a2c to 93cced8 Compare April 9, 2015 18:54
@WalterBright
Copy link
Member Author

Fixed so it has 100% unittest coverage.

@JakobOvrum
Copy link
Member

We have join/joiner, split/splitter etc. when we have both eager and lazy versions of an algorithm. Would this convention be applicable here? capitalizer? toCapitalized?

}

///
@safe pure unittest
Copy link
Member

Choose a reason for hiding this comment

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

toCapitalize looks like it should also be @nogc if used with representation, byChar (or is it byCodeUnit? std.utf is a mess) or handle; the original capitalize can't be tested to be @nogc-ready because of its return value, but this algorithm definitely should be.

edit:

Not suggesting that the example test should be used to verify it, but it should be verified somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

the unit test allocates

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and we need to have one that doesn't.

@MartinNowak
Copy link
Member

Yeah, capitalize/capitalizer makes sense to me, we should strive for consistency here.

@WalterBright
Copy link
Member Author

There is no consistency, and it's a little late to impute one. toCapitalize was named in the same spirit as its brethren toUpperCase and toLowerCase.

@JakobOvrum
Copy link
Member

toCapitalize was named in the same spirit as its brethren toUpperCase and toLowerCase.

Which were also names going against Phobos precedent. There is only a little consistency, but at least it is there - let's not create yet another naming convention just for the heck of it.

@MartinNowak
Copy link
Member

Ah OK, toCapitalizer is only the private impl, toCapitalize works for me.

@JakobOvrum
Copy link
Member

I would have preferred capitalizer, lowerCaser, upperCaser... but it's just a consistency issue, I agree these names are perfectly apt in a vacuum.

@MetaLang
Copy link
Member

toCapitalize doesn't make any sense. It's not proper English. You don't change a letter "to capitalize", you change it "to capital" or you "capitalize" it.

@andralex
Copy link
Member

toCapitalize is not a phrase and I think it needs to be changed before this gets pulled

@WalterBright
Copy link
Member Author

Bill wants to capitalize a sentence.

@andralex
Copy link
Member

Yeah "to" has many meanings which actually makes it a worse choice here - you want the preposition meaning transformation akin to "into", not the function word accompanying an infinitive.

Good choices: toCapitals(), capitalizer(), toCapitalLetters()
Worse choices: crap(), whocares(), meh()

@WalterBright
Copy link
Member Author

toUpperCase() - Bill wants to upper case a string.
toLowerCase() - Bob wants to lower case a string.
toCapitalize() - Fred wants to capitalize a string.

@JakobOvrum
Copy link
Member

This also doesn't have a @nogc unittest yet.

@MetaLang
Copy link
Member

In "a sentence".toCapitalize, Fred is not the subject. There is no Fred. A rough English translation would be (Convert) the characters "a sentence" to capitalize which obviously doesn't make any sense. It's imperative form, where the subject is implicitly "the computer". Please change it.

@andralex
Copy link
Member

toUpperCase() - Bill wants to upper case a string.
toLowerCase() - Bob wants to lower case a string.

I think you are wrong about the use of "to" here and in fact it's the first time I'm seeing this interpretation. It's:

toUpperCase() - Convert this string to uppercase.
toLowerCase() - Convert this string to lowercase.

@MartinNowak
Copy link
Member

Let's settle on toCapitals then.

@WalterBright WalterBright changed the title add std.uni.toCapitalize() add std.uni.toCapitalized() Apr 17, 2015
@WalterBright
Copy link
Member Author

  1. changed to toCapitalized()
  2. added nothrow @nogc unittest

@MetaLang
Copy link
Member

LGTM

@MartinNowak
Copy link
Member

Auto-merge toggled on

MartinNowak added a commit that referenced this pull request Apr 18, 2015
@MartinNowak MartinNowak merged commit c59966e into dlang:master Apr 18, 2015
@WalterBright WalterBright deleted the toCapitalize branch April 18, 2015 00:56
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.

7 participants