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

Word truncation #16190

Merged
merged 1 commit into from Jul 16, 2014
Merged

Word truncation #16190

merged 1 commit into from Jul 16, 2014

Conversation

oss92
Copy link
Contributor

@oss92 oss92 commented Jul 16, 2014

Patched the truncate strings method to include truncation by word

Examples:

'Once upon a time'.truncate(4, word: true)
=> "Once upon a time"

'Once upon a time in a world far far away'.truncate(4, word: true)
=> "Once upon a time..."

'Once<br>upon<br>a<br>time<br>in<br>a<br>world'.truncate(5, word: true, separator: '<br>')
 => "Once<br>upon<br>a<br>time<br>in..."

'Once<br>upon<br>a<br>time<br>in<br>a<br>world'.truncate(5, word: true, separator: '<br>', omission: '[...]')
 => "Once<br>upon<br>a<br>time<br>in[...]"

@rafaelfranca
Copy link
Member

rafaelfranca commented Jul 16, 2014

Can we implement another method instead of adding a new option?

@oss92
Copy link
Contributor Author

oss92 commented Jul 16, 2014

yes we can, I will implement it an create a new pull request

@matthewd
Copy link
Member

matthewd commented Jul 16, 2014

You can just update this pull request; no need for a new one.

If we're going to add such a method, hopefully we can make it a bit more performant, too... this sounds like it would be allocating a lot of objects.

@oss92
Copy link
Contributor Author

oss92 commented Jul 16, 2014

okay will do. Thanks for the advice regarding the pull request

@oss92
Copy link
Contributor Author

oss92 commented Jul 16, 2014

Done and tested, less objects allocated and everything is working fine.

Also added rtruncate to get the rest of a truncated string. This is useful in web apps when truncating strings and having a 'read more...' link, the rest of the string is now easily gotten.

Example
'Once upon a time in a world far far away'.truncate(27, omission: '') + 'Once upon a time in a world far far away'.rtruncate(27, omission: '')
=> "Once upon a time in a world far far away"

#
# 'Once upon a time in a world far far away'.truncate(27, omission: '') + 'Once upon a time in a world far far away'.rtruncate(27, omission: '')
# # => "Once upon a time in a world far far away"
def rtruncate(truncate_at, options = {})
Copy link
Member

@matthewd matthewd Jul 16, 2014

Choose a reason for hiding this comment

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

👎

Copy link
Member

@matthewd matthewd Jul 16, 2014

Choose a reason for hiding this comment

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

I don't think it's sufficiently useful.

(Also, the '(Read more) ...' example seems at best misleading... if you're passing a different omission than you gave to truncate, things are surely going to get confusing?)

Copy link
Contributor Author

@oss92 oss92 Jul 16, 2014

Choose a reason for hiding this comment

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

The read more part is just an example, yes it may be a bit misleading for some developers but the method is useful for, as I said, getting the rest of the truncated string without having to sub! two strings from each other.

Copy link
Member

@matthewd matthewd Jul 16, 2014

Choose a reason for hiding this comment

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

Yeah, I understand the use case... the problem is just that adding a new method to AS requires a pretty high standard of justification, and (personally) I don't think this clears it.

Copy link
Contributor Author

@oss92 oss92 Jul 16, 2014

Choose a reason for hiding this comment

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

Okay, after all it is off-point from the 'word truncation' so it can be removed

@oss92
Copy link
Contributor Author

oss92 commented Jul 16, 2014

The documentation is identical to that of truncate but that's for consistency. Shall I change these lines or do you think the rtruncate method is not at all useful?

split(options[:separator])[0...words_count].join(options[:separator])
else
split[0...words_count].join(' ')
end
Copy link
Member

@matthewd matthewd Jul 16, 2014

Choose a reason for hiding this comment

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

The split here is still allocating a separate string for every word... what about something like:

def truncate_words(words_count, options={})
  sep = options[:separator] || /(?<=\w)\b/
  sep = Regexp.escape(sep.to_s) unless Regexp === sep
  if self =~ /^((?:.+?#{sep}){#{words_count - 1}}.+?)#{sep}.*/
    $1 + (options[:omission] || '...')
  else
    dup
  end
end

Copy link
Contributor Author

@oss92 oss92 Jul 16, 2014

Choose a reason for hiding this comment

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

Yes something like this is much better actually

It just has this problem of a character sticking to the word at the end
For one of the test for my method against yours
Expected: "Hello Big World!"
Actual: "Hello Big World..."

But overall it is much better, it should be the one used because of its efficiency.

Copy link
Member

@matthewd matthewd Jul 16, 2014

Choose a reason for hiding this comment

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

Ah, depending on what you want, try /\s+/ or /\b(?=\w)/.

Or, after building $1 + omission, it could compare the length of the result to decide whether it should still just return dup after all.

Copy link
Contributor Author

@oss92 oss92 Jul 16, 2014

Choose a reason for hiding this comment

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

Yes it works now if
sep = options[:separator] || /\s+/

For me this is better than comparing the length at the end. All tests are fine now. Now, shall I commit this on the same branch or will you create your own pull request with that?

Copy link
Contributor Author

@oss92 oss92 Jul 16, 2014

Choose a reason for hiding this comment

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

Pardon me I still have limited experience with "open-source"

Copy link
Member

@matthewd matthewd Jul 16, 2014

Choose a reason for hiding this comment

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

Yep, go ahead and update with that... and then squash down to a single commit, and add a CHANGELOG entry.

Then we can see whether anyone else has a 👍/👎 opinion on adding the new method. 😄

@oss92
Copy link
Contributor Author

oss92 commented Jul 16, 2014

Ready to merge 😄

@matthewd matthewd merged commit a9d3b77 into rails:master Jul 16, 2014
matthewd added a commit that referenced this pull request Jul 16, 2014
@matthewd
Copy link
Member

matthewd commented Jul 16, 2014

Thanks!

I moved your changelog entry to the top, and fixed an issue with handling of multi-line strings in my suggested regexp.

After I pushed, I noticed that your git committer name is "root" 😟. You can submit a PR to https://github.com/fxn/rails-contributors/blob/master/app/models/names_manager.rb to fix up your entry on http://contributors.rubyonrails.org/, if you like. 😅

@oss92
Copy link
Contributor Author

oss92 commented Jul 16, 2014

I shall add my commit SHA and my name to this method, correct me if I am mistaken please. Thanks

self.authors_of_special_cased_commits(commit)

@matthewd
Copy link
Member

matthewd commented Jul 16, 2014

👍

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.

None yet

3 participants