Add parameter without removing duplicated value in query string #126

rennerocha · 2019-03-27T02:40:49Z

This should fix #125.

…rs to URL query string

codecov · 2019-03-27T02:44:34Z

Codecov Report

Merging #126 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage    95.3%   95.37%   +0.06%     
==========================================
  Files           7        7              
  Lines         469      476       +7     
  Branches       93       97       +4     
==========================================
+ Hits          447      454       +7     
  Misses         15       15              
  Partials        7        7

Impacted Files	Coverage Δ
w3lib/url.py	`97.98% <100%> (+0.07%)`	⬆️

Gallaecio · 2019-03-27T04:55:38Z

@rennerocha Could you add tests for the case where the key you are adding or replacing already exists among the parameters? Specially when it exists twice in the existing URL. I think only the first match is replaced, I’m not sure whether or not that is intended.

rennerocha · 2019-03-27T11:37:09Z

You are right: add_or_replace_parameters("http://domain/test?arg1=v1&arg2=v2&arg1=v3", {"arg1": "v4") will return http://domain/test?arg1=v4&arg2=v2&arg1=v3 .

Not sure what should be the correct behavior. Another option is remove the duplicates and return http://domain/test?arg1=v4&arg2=v2, but I believe that removing existing parameters is exactly what we want to avoid.

Any other thoughts?

Gallaecio · 2019-03-27T13:37:52Z

Well, given the name of the function and the previous behavior, I suspect that we should remove all existing parameters matching the specified name.

rennerocha · 2019-03-27T15:08:13Z

@Gallaecio Did the change as you suggested. The only drawback is that it will not be possible to add duplicated values using this function, as the input is a dictionary.

ejulio · 2019-07-03T20:53:42Z

w3lib/url.py

+            del params[name]
+        elif name not in changing_params:
+            new_args.append((name, value))
+    new_args.extend([(name, value) for name, value in params.items()])


No need for list comprehension here, only new_args.extend(params.items()) is enough.
You can use list(params.items()) to make it explicit.
Also, new_args += params.items() would be more pythonic

As I removed the side-effect over the input parameters, I need this list comprehension to filter the values that I don't want (the modified ones). Replaced .extend to += anyway.

dangra · 2019-07-03T21:06:02Z

w3lib/url.py

-    new_args = OrderedDict(args)
-    new_args.update(params)
+    new_args = []
+    changing_params = list(params.keys())


why is it converted to a list? a set would be more correct and .keys() method call is not required: changing_params = set(params).

dangra · 2019-07-03T21:06:55Z

w3lib/url.py

+    new_args = []
+    changing_params = list(params.keys())
+    for name, value in args:
+        if name in params.keys():


no need to get the list of .keys(), it should be possible to test like if name in params if params is a dict.

dangra · 2019-07-03T21:10:06Z

w3lib/url.py

+    for name, value in args:
+        if name in params.keys():
+            new_args.append((name, params[name]))
+            del params[name]


I don't think mutating params is a good idea, I'd prefer to flag seen params in another set and ignore them on line 215.

Something in the line of

def _add_or_replace_parameters(url, params): parsed = urlsplit(url) args = parse_qsl(parsed.query, keep_blank_values=True) acc = [] seen = set() for k, v in args: if k in params: if k not in seen: acc.append((k, params[k])) seen.add(k) else: acc.append((k, v)) acc += ((k, v) for k, v in params.items() if k not in seen) query = urlencode(acc) return urlunsplit(parsed._replace(query=query))

I don't think mutating params is a good idea, I'd prefer to flag seen params in another set and ignore them on line 215.

As the input argument is a dict, it makes sense that it shouldn't be modified by the function. I included a test case to avoid this scenario as well.

codecov-io · 2019-07-04T11:47:32Z

Codecov Report

Merging #126 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage    95.3%   95.37%   +0.06%     
==========================================
  Files           7        7              
  Lines         469      476       +7     
  Branches       93       97       +4     
==========================================
+ Hits          447      454       +7     
  Misses         15       15              
  Partials        7        7

Impacted Files	Coverage Δ
w3lib/url.py	`97.98% <100%> (+0.07%)`	⬆️

codecov-io · 2019-07-04T11:47:32Z

Codecov Report

Merging #126 into master will decrease coverage by 0.11%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
- Coverage    95.3%   95.19%   -0.12%     
==========================================
  Files           7        7              
  Lines         469      479      +10     
  Branches       93       98       +5     
==========================================
+ Hits          447      456       +9     
  Misses         15       15              
- Partials        7        8       +1

Impacted Files	Coverage Δ
w3lib/url.py	`97.52% <92.85%> (-0.4%)`	⬇️

rennerocha · 2019-07-04T11:56:52Z

@dangra and @ejulio I did some changes after your comments. Could you please review it again?

Gallaecio · 2019-07-05T10:16:22Z

w3lib/url.py

+        if name in params:
+            if name not in seen_params:
+                new_args.append((name, params[name]))
+            seen_params.add(name)


💄 It does not matter much, but…

Suggested change

seen_params.add(name)

seen_params.add(name)

that's good 💅 :)

Gallaecio · 2019-07-05T10:20:24Z

w3lib/url.py

+            if name not in seen_params:
+                new_args.append((name, params[name]))
+            seen_params.add(name)
+        elif name not in changing_params:


I was checking the coverage data, and I may be missing something, but I think this could be replaced by an else, and the changing_params variable may be unnecessary.

else block should be fine per #126 (comment) and
no need for changing_params either

Gallaecio · 2019-08-08T10:04:50Z

@rennerocha We may have a new release soon 😀. Please let us know if you think you’ll have time to address #126 (comment) in the following days.

rennerocha · 2019-08-08T10:33:24Z

@rennerocha We may have a new release soon grinning. Please let us know if you think you’ll have time to address #126 (comment) in the following days.

Hi! Yes, I will work on this comment in the next days. Hopefully on time for the next release!

rennerocha · 2019-08-08T12:12:17Z

@Gallaecio I removed the nested conditional. I believe it is clearer now. Could you please review it again?

Gallaecio · 2019-09-17T06:23:35Z

@dangra @ejulio Can you guys have a look at the latest changes, see if this is ready to merge?

kuza · 2019-10-07T09:45:54Z

Hi guys @dangra @ejulio @Gallaecio, how about merge this pull?

rennerocha added 2 commits March 26, 2019 23:37

Avoid removing duplicated valueswhen adding or replacing new paramete…

8257ab2

…rs to URL query string

Remove unused import

dbe9267

Replace duplicated values when requested

c21da61

ejulio reviewed Jul 3, 2019

View reviewed changes

dangra reviewed Jul 3, 2019

View reviewed changes

Renne Rocha added 2 commits July 4, 2019 08:41

Do not create unnecessary list

aad3c03

Removed side-effect on input argument

fa37abf

Improved legibility

65cd7ca

Gallaecio approved these changes Jul 5, 2019

View reviewed changes

Gallaecio reviewed Jul 5, 2019

View reviewed changes

Removed nested conditional

c9ce0ba

Gallaecio approved these changes Aug 12, 2019

View reviewed changes

ejulio approved these changes Sep 17, 2019

View reviewed changes

Gallaecio mentioned this pull request Sep 19, 2019

fixed url.add_or_replace_parameters to work with multiple value args #138

Closed

wRAR approved these changes Oct 15, 2019

View reviewed changes

Gallaecio merged commit 165f165 into scrapy:master Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parameter without removing duplicated value in query string #126

Add parameter without removing duplicated value in query string #126

rennerocha commented Mar 27, 2019

codecov bot commented Mar 27, 2019 •

edited

Loading

Gallaecio commented Mar 27, 2019

rennerocha commented Mar 27, 2019

Gallaecio commented Mar 27, 2019

rennerocha commented Mar 27, 2019

ejulio Jul 3, 2019

rennerocha Jul 4, 2019

dangra Jul 3, 2019

dangra Jul 3, 2019

dangra Jul 3, 2019

dangra Jul 3, 2019

rennerocha Jul 4, 2019

codecov-io commented Jul 4, 2019

codecov-io commented Jul 4, 2019 •

edited by codecov bot

Loading

rennerocha commented Jul 4, 2019

Gallaecio Jul 5, 2019

dangra Jul 8, 2019

Gallaecio Jul 5, 2019 •

edited

Loading

dangra Jul 8, 2019

Gallaecio commented Aug 8, 2019

rennerocha commented Aug 8, 2019

rennerocha commented Aug 8, 2019

Gallaecio commented Sep 17, 2019

kuza commented Oct 7, 2019

Add parameter without removing duplicated value in query string #126

Add parameter without removing duplicated value in query string #126

Conversation

rennerocha commented Mar 27, 2019

codecov bot commented Mar 27, 2019 • edited Loading

Codecov Report

Gallaecio commented Mar 27, 2019

rennerocha commented Mar 27, 2019

Gallaecio commented Mar 27, 2019

rennerocha commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 4, 2019

Codecov Report

codecov-io commented Jul 4, 2019 • edited by codecov bot Loading

Codecov Report

rennerocha commented Jul 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gallaecio Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gallaecio commented Aug 8, 2019

rennerocha commented Aug 8, 2019

rennerocha commented Aug 8, 2019

Gallaecio commented Sep 17, 2019

kuza commented Oct 7, 2019

codecov bot commented Mar 27, 2019 •

edited

Loading

codecov-io commented Jul 4, 2019 •

edited by codecov bot

Loading

Gallaecio Jul 5, 2019 •

edited

Loading