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

Improve performance of determine_where_to_break_line #1041

Closed
betesh opened this issue Jul 25, 2017 · 3 comments
Closed

Improve performance of determine_where_to_break_line #1041

betesh opened this issue Jul 25, 2017 · 3 comments

Comments

@betesh
Copy link
Contributor

betesh commented Jul 25, 2017

I have the following line of code in my model:

validates :content, length: { maximum: 20.megabytes }

and the following test:

it { should validate_length_of(:content).is_at_most(19.megabytes) }

The process hangs while it's generating the failure message. Using a debugger, I traced it to this line:
https://github.com/thoughtbot/shoulda-matchers/blob/v3.1.2/lib/shoulda/matchers/util/word_wrap.rb#L194

When dealing with very long lines, we should limit how far we go trying to find some whitespace.

What do you think of changing this line from:

while line[index] !~ /\s/ && (0...line.length).cover?(index)

to:

line_length_or_reasonable_limit = [line.length, REASONABLE_LIMIT].min
while line[index] !~ /\s/ && (0...line_length_or_reasonable_limit).cover?(index)

and what would be the REASONABLE_LIMIT (I'm leaning towards 500)?

@mcmire
Copy link
Collaborator

mcmire commented Jul 25, 2017

Thanks for hunting this down, @betesh! It makes complete sense to limit the loop like that (I feel bad now that I didn't do this). We have tests for the word wrap code here. Would you implement the fix you described and write a test for it? I'll happily merge it in.

betesh added a commit to betesh/shoulda-matchers that referenced this issue Jul 25, 2017
@betesh
Copy link
Contributor Author

betesh commented Jul 25, 2017

I took a different approach than I had originally expected. It looks like rspec truncates the message, so the message being displayed is still incomplete. I haven't tracked down the code that does that but I don't think it's in shoulda-matchers.

So I made just one change that addressed the performance issue here. I think that's all we need to do here.

@mcmire
Copy link
Collaborator

mcmire commented May 6, 2020

Hey folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the shoulda-matchers team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this issue.

Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this issue, then please let us know so that we can reprioritize it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants