Changes between Version 7 and Version 10 of Ticket #7086


Ignore:
Timestamp:
11/09/2018 10:33:56 PM (13 months ago)
Author:
kzar
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #7086

    • Property Cc kzar added
    • Property Priority changed from Unknown to P2
    • Property Ready set
    • Property Type changed from defect to change
  • Ticket #7086 – Description

    v7 v10  
    11It seems like how to handle unknown special comments was never defined somehow. 
    22 
    3 In this commit for issue #6923: https://gitlab.com/eyeo/adblockplus/adblockpluscore/commit/ee3158dca717452047320cb69a80a59513997c71 
     3In [https://gitlab.com/eyeo/adblockplus/adblockpluscore/commit/ee3158dca717452047320cb69a80a59513997c71 this commit] for #6923 there was [https://gitlab.com/eyeo/adblockplus/adblockpluscore/commit/ee3158dca717452047320cb69a80a59513997c71#8525ff79982a26589c57c020069186faa286b695_163_163 a change made to a regular expression]. By changing a `\w+` bit for a `.*?` bit, for which no tests were added. 
    44 
    5 there was a change made to a regular expression here: https://gitlab.com/eyeo/adblockplus/adblockpluscore/commit/ee3158dca717452047320cb69a80a59513997c71#8525ff79982a26589c57c020069186faa286b695_163_163 
     5The change adds support for special comments like ` !  : blah` or ` ! *** : foo` by simply ignoring it, this comment previously would've been treated as a filter rule and any special comments after it would've been treated as a filter rule too. 
    66 
    7 by changing a `\w+` bit for a `.*?` bit, for which no tests were added. 
    8  
    9 The change adds support for special comments like ` !  : blah` or ` ! *** : foo` by simply ignoring it, this comment previously would've been treated as a filter rule and any special comments after it would've been treated as a filter rule too.  I'm not saying it's a bad change, but it is a change for which no test was added, so if the change was intentional then we should add a test for this.  
    10  
    11 Also, I'm wondering why the change was made?  because if this is a use case that we want to support then I'm not sure why we would want to support the use case.  If it's not then we should also have a test that we do not support this use case. 
     7Let's at least add some unit tests for this.