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

Truncate subject within begin and end with assertions #320

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@alexjeffburke
Collaborator

alexjeffburke commented Jul 12, 2016

Hey,

This is my first attempt at doing this - feedback both requested and very welcome.

I might have gone a little overboard, the the current implementation details are as follows:
This commit uses 25 characters of context beyond the length of the prefix/suffix respectively. Should the subject be longer than the needle + 25, it is shortened. I implemented a slightly more intelligent truncation where it searches those additional 25 characters for a space and breaks there if found. The partialMatch is also updated to show a truncated version of the subject.

I'm conscious this is my first submission to a core assertion and am more than willing to rework this. Looking forward to your thoughts.

AJB

@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Jul 12, 2016

Member

It would be nice if the ellipsis could be placed outside the singlequotes so you can easily tell the difference between the string "abc..." and a string that was truncated after abc?

Member

papandreou commented Jul 12, 2016

It would be nice if the ellipsis could be placed outside the singlequotes so you can easily tell the difference between the string "abc..." and a string that was truncated after abc?

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Jul 12, 2016

Collaborator

That's relatively easily doable in the subject, but in the partial match case I chose to include the 'ellipsis' to reinforce the truncation:

foobarbaz-ButAtSomePointThisStringW...
^^^^^^

and with no quotes there I'm not sure what to do?

Collaborator

alexjeffburke commented Jul 12, 2016

That's relatively easily doable in the subject, but in the partial match case I chose to include the 'ellipsis' to reinforce the truncation:

foobarbaz-ButAtSomePointThisStringW...
^^^^^^

and with no quotes there I'm not sure what to do?

@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Jul 12, 2016

Member

That's in the diff, right? I think it's much more tolerable that you can't necessarily tell the difference there than in the subject rendering where it's in quotes and formatted as a JS string literal.

In ansi mode we can make the ellipsis gray in the diff, that could help a bit.

Member

papandreou commented Jul 12, 2016

That's in the diff, right? I think it's much more tolerable that you can't necessarily tell the difference there than in the subject rendering where it's in quotes and formatted as a JS string literal.

In ansi mode we can make the ellipsis gray in the diff, that could help a bit.

@Munter

This comment has been minimized.

Show comment
Hide comment
@Munter

Munter Jul 12, 2016

Member

If I want to assert on compressed source code there might be a long way between spaces. Maybe more characters should be used as delimiters

Member

Munter commented Jul 12, 2016

If I want to assert on compressed source code there might be a long way between spaces. Maybe more characters should be used as delimiters

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Jul 12, 2016

Collaborator

@papandreou
Ooh nice touch :) I guess that'd mean printing an ellipsis separately with a special pen command?

Yeah, I see what you mean - just wanted to make sure that the slight different placement of the ellipsis wouldn't be an issue. Btw, do you prefer three actual dots or the ellipsis character?

@Munter thought that exact thing too and almost added semicolon, but I was worried about enough magic to undermine a common case. Definitely a point for discussion by more eyes!

Collaborator

alexjeffburke commented Jul 12, 2016

@papandreou
Ooh nice touch :) I guess that'd mean printing an ellipsis separately with a special pen command?

Yeah, I see what you mean - just wanted to make sure that the slight different placement of the ellipsis wouldn't be an issue. Btw, do you prefer three actual dots or the ellipsis character?

@Munter thought that exact thing too and almost added semicolon, but I was worried about enough magic to undermine a common case. Definitely a point for discussion by more eyes!

@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Jul 12, 2016

Member

This will try to truncate a string before a space if it can find one at positions 11-20, and fall back to outputting the first 20 chars otherwise (I picked the numbers arbitrarily):

var truncated = string.replace(/^(.{10,19}\S\b|.{20}).*$/, '$1');
'yadda foobarquux'       => 'yadda foobarquux'
'yadda foobarquux baz'   => 'yadda foobarquux baz'
'yadda foobarquux baaz'  => 'yadda foobarquux'
'yadda foobarquuxbaaaaz' => 'yadda foobarquuxbaaa'
Member

papandreou commented Jul 12, 2016

This will try to truncate a string before a space if it can find one at positions 11-20, and fall back to outputting the first 20 chars otherwise (I picked the numbers arbitrarily):

var truncated = string.replace(/^(.{10,19}\S\b|.{20}).*$/, '$1');
'yadda foobarquux'       => 'yadda foobarquux'
'yadda foobarquux baz'   => 'yadda foobarquux baz'
'yadda foobarquux baaz'  => 'yadda foobarquux'
'yadda foobarquuxbaaaaz' => 'yadda foobarquuxbaaa'
Show outdated Hide outdated lib/assertions.js Outdated
Show outdated Hide outdated lib/assertions.js Outdated
@sunesimonsen

This comment has been minimized.

Show comment
Hide comment
@sunesimonsen

sunesimonsen Jul 15, 2016

Member

@alexjeffburke this is a really good start, I really like the result that I can see in the tests. I have added some line comments about things we should consider visiting. It would be nice to move this functionality to a lib/utils.js function and maybe make use of @papandreou's regular expression. The function can just return an object with the needed values.

Member

sunesimonsen commented Jul 15, 2016

@alexjeffburke this is a really good start, I really like the result that I can see in the tests. I have added some line comments about things we should consider visiting. It would be nice to move this functionality to a lib/utils.js function and maybe make use of @papandreou's regular expression. The function can just return an object with the needed values.

@sunesimonsen

This comment has been minimized.

Show comment
Hide comment
@sunesimonsen

sunesimonsen Oct 3, 2016

Member

@alexjeffburke this seems almost ready - but it would be nice if you could consider using something like @papandreou's regexp and move the actual string truncation to utils if possible.

Member

sunesimonsen commented Oct 3, 2016

@alexjeffburke this seems almost ready - but it would be nice if you could consider using something like @papandreou's regexp and move the actual string truncation to utils if possible.

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Sep 1, 2018

Collaborator

This PR is replaced by #504.

Collaborator

alexjeffburke commented Sep 1, 2018

This PR is replaced by #504.

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