Repository: karlicoss/promnesia

Issue:
Handle query parameters without values
Right now, pages on isfdb like `http://www.isfdb.org/cgi-bin/title.cgi?2172` are canonicalized to `http://www.isfdb.org/cgi-bin/title.cgi`, which removes important info from the URL.

I made a somewhat hacky solution, but there is perhaps a more elegant way. The solution is in two parts:

First, modify `Spec` to accept `qkeep=True`, which is taken to mean "retain all query parameters", and have isfdb.org use that option.

```
@@ -105,11 +105,13 @@ default_qkeep = [
 
 # TODO perhaps, decide if fragment is meaningful (e.g. wiki) or random sequence of letters?
 class Spec(NamedTuple):
-    qkeep  : Optional[Collection[str]] = None
+    qkeep  : Optional[Union[Collection[str], bool]] = None
     qremove: Optional[Set[str]] = None
     fkeep  : bool = False
 
     def keep_query(self, q: str) -> Optional[int]: # returns order
+        if self.qkeep is True:
+            return 1
         qkeep = {
             q: i for i, q in enumerate(chain(default_qkeep, self.qkeep or []))
         }
@@ -183,6 +185,7 @@ specs: Dict[str, Spec] = {
     'ycombinator.com'    : S(qkeep={'id'}), # todo just keep id by default?
     'play.google.com'    : S(qkeep={'id'}),
     'answers.yahoo.com'  : S(qkeep={'qid'}),
+    'isfdb.org': S(qkeep=True),
 }
 _def_spec = S()
```

Second, pass, `keep_blank_values=True` to `parse_qsl`.

```
@@ -271,7 +274,7 @@ def transform_split(split: SplitResult):
     netloc = canonify_domain(split.netloc)
 
     path     = split.path
-    qparts   = parse_qsl(split.query)
+    qparts   = parse_qsl(split.query, keep_blank_values=True)
 
     fragment = split.fragment
 
@@ -319,7 +322,7 @@ def transform_split(split: SplitResult):
             to = to + ('', )
 
         (netloc, path, qq) = [t.format(**gd) for t in to]
-        qparts.extend(parse_qsl(qq)) # TODO hacky..
+        qparts.extend(parse_qsl(qq, keep_blank_values=True)) # TODO hacky..
         # TODO eh, qparts should really be a map or something...
         break
```

This achieves the desired result, but it's unpleasantly hacky.

First problem: this will not guarantee the order of query parameters. But is that important? Why doesn't the code just always alphabetize them? There's a note for youtube that "order matters here", but it seems to me that youtube doesn't care what order the parameters are in.

Second problem: maybe you usually don't want empty parameters kept. It could complicate things to add logic around this, but maybe something like a `keep_blank_values` property on Spec would do the job.

Third problem: it's not great to have to dig around in the program code to fix this behavior. Maybe it could be patched in from my `config.py`, once the supporting code is in place, which something like:

```python
from promnesia.cannon import specs, S
specs['isfdb.org'] = S(qkeep=True)
```

Really, all of `cannon.py` is a bit intimidating to modify (and, incidentally, shouldn't it be `canon.py`?). I'm not sure what's the right direction to go.

Discussion:

Patch:
diff --git a/src/promnesia/cannon.py b/src/promnesia/cannon.py
index a3a303f..b76cb9f 100755
--- a/src/promnesia/cannon.py
+++ b/src/promnesia/cannon.py
@@ -105,11 +105,13 @@ default_qkeep = [
 
 # TODO perhaps, decide if fragment is meaningful (e.g. wiki) or random sequence of letters?
 class Spec(NamedTuple):
-    qkeep  : Optional[Collection[str]] = None
+    qkeep  : Optional[Union[Collection[str], bool]] = None
     qremove: Optional[Set[str]] = None
     fkeep  : bool = False
 
     def keep_query(self, q: str) -> Optional[int]: # returns order
+        if self.qkeep is True:
+            return 1
         qkeep = {
             q: i for i, q in enumerate(chain(default_qkeep, self.qkeep or []))
         }
@@ -183,6 +185,7 @@ specs: Dict[str, Spec] = {
     'ycombinator.com'    : S(qkeep={'id'}), # todo just keep id by default?
     'play.google.com'    : S(qkeep={'id'}),
     'answers.yahoo.com'  : S(qkeep={'qid'}),
+    'isfdb.org': S(qkeep=True),
 }
 
 _def_spec = S()
@@ -271,7 +274,7 @@ def transform_split(split: SplitResult):
     netloc = canonify_domain(split.netloc)
 
     path     = split.path
-    qparts   = parse_qsl(split.query)
+    qparts   = parse_qsl(split.query, keep_blank_values=True)
 
     fragment = split.fragment
 
@@ -319,7 +322,7 @@ def transform_split(split: SplitResult):
             to = to + ('', )
 
         (netloc, path, qq) = [t.format(**gd) for t in to]
-        qparts.extend(parse_qsl(qq)) # TODO hacky..
+        qparts.extend(parse_qsl(qq, keep_blank_values=True)) # TODO hacky..
         # TODO eh, qparts should really be a map or something...
         break
 


Test patch:
diff --git a/tests/cannon.py b/tests/cannon.py
index d76693b..5cd6ef5 100644
--- a/tests/cannon.py
+++ b/tests/cannon.py
@@ -294,3 +294,19 @@ def test_error():
     with pytest.raises(CanonifyException):
         # borrowed from https://bugs.mageia.org/show_bug.cgi?id=24640#c7
         canonify('https://example.com\uFF03@bing.com')
+
+@pytest.mark.parametrize("url,expected", [
+    ('https://news.ycombinator.com/item?id=', 'news.ycombinator.com/item?id='),
+    ('https://www.youtube.com/watch?v=hvoQiF0kBI8&list&index=2',
+     'youtube.com/watch?v=hvoQiF0kBI8&list='),
+])
+def test_empty_query_parameter(url, expected):
+    assert canonify(url) == expected
+
+@pytest.mark.parametrize("url,expected", [
+    ('http://www.isfdb.org/cgi-bin/title.cgi?2172', 'isfdb.org/cgi-bin/title.cgi?2172='),
+    ('http://www.isfdb.org/cgi-bin/title.cgi?2172+1', 'isfdb.org/cgi-bin/title.cgi?2172%201='),
+    ('http://www.isfdb.org/cgi-bin/title.cgi?2172&foo=bar&baz&quux', 'isfdb.org/cgi-bin/title.cgi?2172=&baz=&foo=bar&quux='),
+])
+def test_qkeep_true(url, expected):
+    assert canonify(url) == expected