Updates should not ignore items with links #41

mtodd · 2014-09-25T03:42:04Z

A regression as part of #38 is that items followed immediately by links get ignored on update. Whoops. Here's a failing test to reproduce the user reported issue so we can work towards a fix.

cc @github/task-lists @rsese @aroben

mtodd · 2014-09-25T03:48:55Z

Could use some regexp help... struggling to distinguish when to update between these scenarios:

- [ ] one
- [ ] [two] # skip this
- [ ] (three) # and skip this
- [ ] [four] (url) # but not this
- [ ] [five] [ref] # nor this
- [ ] six

aroben · 2014-09-25T16:35:54Z

Do you know what line of code is failing?

mtodd · 2014-09-25T17:16:54Z

@aroben essentially, this stanza of the itemPattern helps us skip over list items that look like task list items but are links:

task_list/app/assets/javascripts/task_list.coffee

Lines 128 to 131 in b3b4514

    
             (?!                     # is not a link 
        
               \s*                   # with optional whitespace 
        
               (?:\(.*?\)|\[.*?\])   # because of destination or reference 
        
             )

But it needs to look further ahead to make sure that it isn't a task list followed by a link.

Feels like a fools errand. Might be worth splitting up the pattern matching to have an explicit ignore step that can better handle this kind of matching.

mtodd · 2014-09-25T17:19:06Z

Here's a scriptular reproduction to play with: http://bit.ly/ZePF7z

aroben · 2014-09-25T18:20:33Z

Seems like we want to skip even sequences of square bracket pairs. I.e.:

Skip:

- [ ] [ ]
- [ ] [ ] [ ] [ ]
- [ ] [ ] [ ] [ ] [ ] [ ]

Don't skip:

- [ ]
- [ ] [ ] [ ]
- [ ] [ ] [ ] [ ] [ ]

I'm sure there's some regex-fu that would do this given that JS regexes support back references.

mtodd · 2014-09-25T20:22:30Z

@aroben heh, that's a good observation. Will play around with that.

This is particularly frustrating since we're having to ignore what's essentially a mistake, - [ ] (just parenthetical) being turned into a link by Markdown.

In pseudocode, I'm thinking: match item, but not if it's actually a link, unless that link is a independent and separate from the checkbox. The wording may be confusing, but it's the "not...unless" part that seems intractable right now.

aroben · 2014-09-25T20:29:05Z

I think of it like this:

We match if:

It is [ ] or [x], and
it is not followed by \s*(.*) (because that would mean it itself is a link), and
it is followed by an even number of \s*[.*] pairs (including 0 pairs) (because that means that there are some reference-style links that follow it that the initial brackets are not part of).

1 and 2 are easy. 3 is a little tricky, but I think doable.

mtodd · 2014-09-25T21:20:19Z

@aroben number 2 is incomplete since [ ] [ref] is also a valid link, which makes it harder to account for I think, or at least harder to reason about.

aroben · 2014-09-25T21:32:55Z

Item 3 handles that case. The item must be followed by an even number of [] pairs. In your example it's followed by 1 [] pair.

-Adam

On Sep 25, 2014, at 5:20 PM, Matt Todd [email protected] wrote:

@aroben number 2 is incomplete since [ ] [ref] is also a valid link, which makes it harder to account for I think, or at least harder to reason about.

—
Reply to this email directly or view it on GitHub.

mtodd · 2014-09-25T22:19:46Z

@aroben 👍 good point. Taking another swing at it now.

aroben · 2014-09-25T22:36:33Z

I think I was wrong about needing back references before. I think you can use a negative lookahead assertion to handle the ()-style links and a positive lookahead assertion to handle the []-style links. Something like:

///
  # the good stuff goes here
  (?!\s*\(.*?\)) # Not a [foo](url) link
  (?=
    \s*
    (?:\[.*?\]\s*){2}* # Must be followed by an even number of [] pairs (including 0)
    (?:[^[]|$)         # After all the pairs comes either something that's not a [, or the end of the line
  )
///

@aroben

Still fails, though. cc @aroben

aroben · 2014-09-25T23:41:11Z

app/assets/javascripts/task_list.coffee

-  (?!                     # is not a link
-    \s*                   # with optional whitespace
-    (?:\(.*?\)|\[.*?\])   # because of destination or reference
-  )
  (?=\s)                  # is followed by whitespace


This probably needs to be \s+ so that there can be more than one whitespace character.

Oh I guess extra whitespace characters don't need to be matched here. They'll just be after the pattern (or consumed by \s* in the subsequent assertions).

Does this need to be a lookahead assertion at all? Would a bare \s work not enclosed in (?=)? (This may be unrelated to fixing this bug.)

@aroben lookahead probably isn't needed. Agreed on the extra whitespace not needing to be matched here. Unrelated to the issue, I believe.

Actually I think removing the (?=) makes the test pass. Give it a try!

I think the problem is that when you have two positive lookahead assertions in a row they both need to match. Instead we want the \s to match and then the reference stuff to come after that.

mtodd · 2014-09-25T23:47:09Z

I'm to the point where I think it'd just be better to only handle the [ ] (*) case and punt on [ ] [*] case until we can revisit it. The former seems to be what's most affected in the real world.

Punting on proper handling for now.

mtodd · 2014-09-25T23:55:36Z

@aroben this is what I have right now, and it's still failing:

diff --git a/app/assets/javascripts/task_list.coffee b/app/assets/javascripts/task_list.coffee
index f5cf946..7fec188 100644
--- a/app/assets/javascripts/task_list.coffee
+++ b/app/assets/javascripts/task_list.coffee
@@ -125,11 +125,21 @@ itemPattern = ///
     #{escapePattern(complete)}|
     #{escapePattern(incomplete)}
   )
-  (?=\s)                  # is followed by whitespace
+  \s                  # is followed by whitespace
   (?!                     # but not a link to this destination
     \s*
     \(.*?\)
   )
+  (?=                     # and not part of a link reference
+    \s*
+    (?:(?:\[.*?\]\s*){2})*
+    (?:[^\[]|$)
+  )
 ///

 # Used to filter out code fences from the source for comparison only.

Expected:   
"- [ ] [link label](link)
- [x] [reference label][reference]"
Result:     
"- [ ] [link label](link)
- [ ] [reference label][reference]"
Diff:   
 "-  [  ]  [link  label](link)
 - [x] [ ]  [reference  label][reference]"

aroben · 2014-09-26T00:08:33Z

Hm, strange. In both Safari and Chrome's console things seem to work the way we want:

/^(?:\s*(?:>\s*)*(?:[-+*]|(?:\d+\.)))\s*(\[\s\]|\[[xX]\])\s(?!\s*\(.*?\))(?=\s*(?:(?:\[.*?\]\s*){2})*(?:[^\[]|$))/.exec("- [ ] [reference label][reference]")
["- [ ] ", "[ ]"]

…so why isn't it passing the test?

aroben · 2014-09-26T00:10:16Z

Oh, maybe phantomjs has a too-old JS engine.

aroben · 2014-09-26T00:11:04Z

Yup, that's the problem:

phantomjs> /^(?:\s*(?:>\s*)*(?:[-+*]|(?:\d+\.)))\s*(\[\s\]|\[[xX]\])\s(?!\s*\(.*?\))(?=\s*(?:(?:\[.*?\]\s*){2})*(?:[^\[]|$))/.exec("- [ ] [reference label][reference]")
null

aroben · 2014-09-26T00:11:15Z

I wonder if this is fixed in PhantomJS 2 /cc @eanakashima

mtodd · 2014-09-26T00:20:30Z

Reproduced something that works here: http://bit.ly/ZfhL2s

@aroben seems that 765f60f works. Does that make sense to you?

mtodd · 2014-09-26T00:23:10Z

Here's my PhantomJS version locally:

task_list:skip-links-not-items-with-links $ phantomjs -v
1.9.0

It looks like earlier changes are passing in CI (6+ minute long builds for the loss) which makes this sound reasonable.

Hopefully this most recent change will work though.

Update: 6+ minutes, not 20+.

mtodd · 2014-09-26T00:24:54Z

Not loving regexp right meow 😼

aroben · 2014-09-26T00:33:59Z

@mtodd Nice. Good call on allowing the checklist item to be followed by a []() link. I simplified it a teeny bit in 9bd8e69. Feel free to pull that in if you'd like.

mtodd · 2014-09-26T00:35:53Z

@aroben legit, let's see how CI feels about it.

mtodd · 2014-09-26T00:38:57Z

Feeling that the style of testing employed here is masking which items are being ignored, which can lead us astray (pretty sure that happened a few times today). Need to make writing these tests much easier and simpler.

Updates should not ignore items with links

mtodd · 2014-09-26T00:58:53Z

@aroben really appreciate the help here. The brink of sanity is a scary place. ❤️

aroben · 2014-09-26T00:59:48Z

👬

eanakashima · 2014-09-26T07:11:52Z

I wonder if this is fixed in PhantomJS 2

Appears not to be. I also get null instead of ["- [ ] ", "[ ]"]. 😒

Add failing test for items with links

eeb8692

mtodd added 2 commits September 25, 2014 16:32

Split link dest from link reference, more explicit

e103915

Still fails, though. cc @aroben

Move required whitespace earlier

fdca517

aroben reviewed Sep 25, 2014
View reviewed changes

Revert ignoring link references

b628d7a

Punting on proper handling for now.

Got it

765f60f

Simplify the regex a little bit

9bd8e69

mtodd added a commit that referenced this pull request Sep 26, 2014

Merge pull request #41 from github/skip-links-not-items-with-links

b43e109

Updates should not ignore items with links

mtodd merged commit b43e109 into master Sep 26, 2014

mtodd deleted the skip-links-not-items-with-links branch September 26, 2014 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates should not ignore items with links #41

Updates should not ignore items with links #41

mtodd commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

mtodd commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

aroben Sep 25, 2014

aroben Sep 25, 2014

aroben Sep 25, 2014

mtodd Sep 25, 2014

aroben Sep 25, 2014

aroben Sep 25, 2014

mtodd commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 26, 2014

aroben commented Sep 26, 2014

aroben commented Sep 26, 2014

aroben commented Sep 26, 2014

mtodd commented Sep 26, 2014

mtodd commented Sep 26, 2014

mtodd commented Sep 26, 2014

aroben commented Sep 26, 2014

mtodd commented Sep 26, 2014

mtodd commented Sep 26, 2014

mtodd commented Sep 26, 2014

aroben commented Sep 26, 2014

eanakashima commented Sep 26, 2014

Updates should not ignore items with links #41

Updates should not ignore items with links #41

Conversation

mtodd commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

mtodd commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 25, 2014

aroben Sep 25, 2014

Choose a reason for hiding this comment

aroben Sep 25, 2014

Choose a reason for hiding this comment

aroben Sep 25, 2014

Choose a reason for hiding this comment

mtodd Sep 25, 2014

Choose a reason for hiding this comment

aroben Sep 25, 2014

Choose a reason for hiding this comment

aroben Sep 25, 2014

Choose a reason for hiding this comment

mtodd commented Sep 25, 2014

mtodd commented Sep 25, 2014

aroben commented Sep 26, 2014

aroben commented Sep 26, 2014

aroben commented Sep 26, 2014

aroben commented Sep 26, 2014

mtodd commented Sep 26, 2014

mtodd commented Sep 26, 2014

mtodd commented Sep 26, 2014

aroben commented Sep 26, 2014

mtodd commented Sep 26, 2014

mtodd commented Sep 26, 2014

mtodd commented Sep 26, 2014

aroben commented Sep 26, 2014

eanakashima commented Sep 26, 2014