cannon: more reasonable ordering for query elements

Author: karlicoss

src/promnesia/cannon.py

@@ -11,9 +11,10 @@
 """
 # TODO eh?? they fixed mobile.twitter.com?
 
+from itertools import chain
 import re
 import typing
-from typing import Iterable, NamedTuple, Set, Optional, List, Sequence, Union, Tuple, Dict, Any
+from typing import Iterable, NamedTuple, Set, Optional, List, Sequence, Union, Tuple, Dict, Any, Collection
 
 import urllib.parse
 from urllib.parse import urlsplit, parse_qsl, urlunsplit, parse_qs, urlencode, SplitResult
@@ -89,44 +90,42 @@ def canonify_domain(dom: str):
     'utf8',
 }
 
-default_qkeep = {
+default_qkeep = [
     # ok, various BBS have it, hackernews has it etc?
     # hopefully it's a reasonable one to keep..
     'id',
 
-    # common to some sites..
+    # common to forums, usually means 'thread'
+    't',
+
+    # common to some sites.., usually 'post'
     'p',
-}
+]
 
 # TODO perhaps, decide if fragment is meaningful (e.g. wiki) or random sequence of letters?
 class Spec(NamedTuple):
-    qkeep  : Optional[Set[str]] = None
+    qkeep  : Optional[Collection[str]] = None
     qremove: Optional[Set[str]] = None
     fkeep  : bool = False
 
-    def keep_query(self, q: str) -> bool:
-        qkeep   = default_qkeep  .union(self.qkeep or {})
+    def keep_query(self, q: str) -> Optional[int]: # returns order
+        qkeep = {
+            q: i for i, q in enumerate(chain(default_qkeep, self.qkeep or []))
+        }
         qremove = default_qremove.union(self.qremove or {})
         # I suppose 'remove' is only useful for logging. we remove by default anyway
 
         keep = False
         remove = False
-        if qkeep is not None and q in qkeep:
-            keep = True
-        # pylint: disable=unsupported-membership-test
+        qk = qkeep.get(q)
+        if qk is not None:
+            return qk
+        # todo later, check if spec tells both to keep and remove?
         if q in qremove:
-            remove = True
-        if keep and remove:
-            # keep wins?
-            return True # TODO need a warning
-        if keep:
-            return True
-        if remove:
-            return False
-
+            return None
         # by default drop all
         # it's a better default, since if it's *too* unified, the user would notice it. but not vice versa!
-        return False
+        return None
 
     @classmethod
     def make(cls, **kwargs):
@@ -138,14 +137,13 @@ def make(cls, **kwargs):
 specs = {
     'youtube.com': S(
         # TODO search_query?
-        qkeep={
-            'list', # TODO hmm. list is kinda important for playlist urls...
+        qkeep=[ # note: experimental.. order matters here
             'v',
-        }, # TODO frozenset?
+            't',
+            'list',
+        ],
+        # todo frozenset?
         qremove={
-            't', # TODO not so sure about it
-
-
             'time_continue', 'index', 'feature', 'lc', 'app', 'start_radio', 'pbjreload', 'annotation_id',
             'flow', 'sort', 'view',
             'enablejsapi', 'wmode', 'html5', 'autoplay', 'ar',
@@ -181,10 +179,9 @@ def make(cls, **kwargs):
     'wikipedia.org': S(fkeep=True),
     'scottaaronson.com'  : S(qkeep={'p'}, fkeep=True),
     'urbandictionary.com': S(qkeep={'term'}),
-    'ycombinator.com'    : S(qkeep={'id'}),
+    'ycombinator.com'    : S(qkeep={'id'}), # todo just keep id by default?
     'play.google.com'    : S(qkeep={'id'}),
     'answers.yahoo.com'  : S(qkeep={'qid'}),
-    'ubuntuforums.org'   : S(qkeep={'t'}),
 }
 
 _def_spec = S()
@@ -410,8 +407,12 @@ def canonify(url: str) -> str:
     # frag = parts.fragment if spec.fkeep else ''
     frag = ''
 
-    qq = [(k, v) for k, v in qq if spec.keep_query(k)]
-    qq = list(sorted(qq)) # order matters in theory, but definitely not by default
+    iqq = []
+    for k, v in qq:
+        order = spec.keep_query(k)
+        if order is not None:
+            iqq.append((order, k, v))
+    qq = [(k, v) for i, k, v in sorted(iqq)]
     # TODO still not sure what we should do..
     # quote_plus replaces %20 with +, not sure if we want it...
     query = urlencode(qq, quote_via=quote_via) # type: ignore[type-var]

tests/cannon.py

@@ -28,9 +28,10 @@ def check(url, expected):
 # then could align URLs etc?
 
 @param('url,expected', [(
-    'https://www.youtube.com/watch?v=1NHbPN9pNPM&index=63&list=WL&t=491s',
-    # TODO not so sure about &t, it's sort of useful
-    'youtube.com/watch?list=WL&v=1NHbPN9pNPM'
+    'https://www.youtube.com/watch?t=491s&v=1NHbPN9pNPM&index=63&list=WL',
+    # NOTE: t= reordered, makes it more hierarchical
+    # list as well, I guess makes the most sense to keep it at the very end.. since lists are more like tags
+    'youtube.com/watch?v=1NHbPN9pNPM&t=491s&list=WL'
 ), (
     'youtube.com/watch?v=wHrCkyoe72U&feature=share&time_continue=6',
     'youtube.com/watch?v=wHrCkyoe72U'
@@ -39,8 +40,7 @@ def check(url, expected):
     'youtube.com/watch?v=nyc6RJEEe0U'
 ), (
     'https://youtu.be/iCvmsMzlF7o?list=WL',
-    # TODO hmm. ordering?
-    'youtube.com/watch?list=WL&v=iCvmsMzlF7o'
+    'youtube.com/watch?v=iCvmsMzlF7o&list=WL'
 ),
     # TODO can even be like that or contain timestamp (&t=)
     # TODO warn if param already present? shouldn't happen..
@@ -196,10 +196,10 @@ def test_reddit(url, expected):
 
     # should sort query params
     ( 'https://www.youtube.com/watch?v=hvoQiF0kBI8&list=WL&index=2'
-    , 'youtube.com/watch?list=WL&v=hvoQiF0kBI8',
+    , 'youtube.com/watch?v=hvoQiF0kBI8&list=WL',
     ),
     ( 'https://www.youtube.com/watch?list=WL&v=hvoQiF0kBI8&index=2'
-    , 'youtube.com/watch?list=WL&v=hvoQiF0kBI8',
+    , 'youtube.com/watch?v=hvoQiF0kBI8&list=WL',
     ),
 
     # TODO def need to allow the _user_ to define the rules.
@@ -211,7 +211,7 @@ def test_reddit(url, expected):
     ),
 
     ( 'https://ubuntuforums.org/showthread.php?t=1403470&s=0dd67bdb12559c22e73a220752db50c7&p=8806195#post8806195'
-    , 'ubuntuforums.org/showthread.php?p=8806195&t=1403470',
+    , 'ubuntuforums.org/showthread.php?t=1403470&p=8806195',
     ),
 
     ( 'https://arstechnica.com/?p=1371299',