Skip to content

To_Title is corrected #372

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

Closed

Conversation

ChetanKarwa
Copy link

The to_title function was not working properly it is working like to_sentence:

input:- this text is some title
earlier output:- This text is some title
expected output:- This Text Is Some Title

I made the necessary changes.
Check it out.

@aman-godara
Copy link
Member

aman-godara commented Mar 31, 2021

Titled version: The first alphabetical character of the input character sequence is transformed to uppercase unless it follows a numeral and the rest of the characters in the sequence are transformed to lowercase.

Please check these comments by arjenmarkus and awvwgk to understand this better : #346 (comment) & #346 (comment)

@ChetanKarwa
Copy link
Author

Ok I get that.
That is what we have implemented and written in description but the common implementation is capitalising each word. (There are exceptional words)
Same is implemented in python language and on various online sentence to title convertors.

I have not checked in other languages but I suggest we should also implement to_title the similar way and the current implementation can be called 'to_sentence' or any other name that suits the operation.

Just a suggestion to land on common ground with other languages.

@jvdp1
Copy link
Member

jvdp1 commented Apr 8, 2021

I have not checked in other languages but I suggest we should also implement to_title the similar way and the current implementation can be called 'to_sentence' or any other name that suits the operation

I didn't think about it while reading the specs. However, I may agree with this comment. @awvwgk what do you think about it?

@awvwgk
Copy link
Member

awvwgk commented Apr 8, 2021

I just checked python strings and they have both .capitalize() and title(), the current to_title implementation is a version of capitalize under the wrong name. to_sentence would be a better name and we can also have an actual to_title function.

@ChetanKarwa Would you adjust your patch to rename the current to_title function as to_sentence and than we can discuss the actual to_title implementation here?

@ChetanKarwa
Copy link
Author

Sure will do that.

@ChetanKarwa
Copy link
Author

For now, the PR holds the to_title() function and so I would go forward and implement the to_sentence() separately so that in case we all come down to the conclusion of implementing to_title() this can be used.

else
title_string(i:i) = string(i:i)
if(string(i:i)==" ") then
Copy link
Member

Choose a reason for hiding this comment

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

This could be more general than just spaces, every non-word boundary might trigger capitalization, consider:
to_title(".this.sentence.is.separated.with.dots."), would we expect .This.sentence.is.separated.with.dots. or .This.Sentence.Is.Separated.With.Dots. for a titlecase return value?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm! I am not sure what to expect in such a case. My favor would go to capitalize each word. What do other languages?

Copy link
Member

Choose a reason for hiding this comment

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

I checked for python and realised that every character which is not an alphabet triggers capitalization including integers. Python mentions that even apostrophe(') triggers this capitalization which is undesirable. May be we can take an optional set as the argument from the user which will consist of characters the user wants to the trigger capitalization for..

@awvwgk awvwgk added the waiting for OP This patch requires action from the OP label Apr 17, 2021
@aman-godara
Copy link
Member

I am assigning this task to myself as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for OP This patch requires action from the OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants