Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+2] Added: Allowing optional arguments for scrapy.http.cookies.CookieJar.clear #3231

Merged

Conversation

@starrify
Copy link
Contributor

@starrify starrify commented Apr 23, 2018

So it matches the signature of the underlying http.cookiejar.CookieJar.clear object.

….clear`
@codecov
Copy link

@codecov codecov bot commented Apr 25, 2018

Codecov Report

Merging #3231 into master will not change coverage.
The diff coverage is 50%.

@@           Coverage Diff           @@
##           master    #3231   +/-   ##
=======================================
  Coverage   82.12%   82.12%           
=======================================
  Files         228      228           
  Lines        9593     9593           
  Branches     1385     1385           
=======================================
  Hits         7878     7878           
  Misses       1456     1456           
  Partials      259      259
Impacted Files Coverage Δ
scrapy/http/cookies.py 89.28% <50%> (ø) ⬆️
@@ -58,8 +58,8 @@ def _cookies(self):
def clear_session_cookies(self, *args, **kwargs):
return self.jar.clear_session_cookies(*args, **kwargs)

def clear(self):
return self.jar.clear()
def clear(self, domain=None, path=None, name=None):

This comment has been minimized.

@kmike

kmike Apr 25, 2018
Member

What do you think about using args/kwargs, like in clear_session_cookies? This way we won't have to change this method if Python decide to change signature for some reason.

This comment has been minimized.

@starrify

starrify Apr 27, 2018
Author Contributor

Currently there're two types of function signatures here:

  • Using args/kwargs, like clear_session_cookies.
  • Using the exact signature of the underlying CookieJar object, like set_cookie and set_policy.

For now I don't think any type has a significant advantage to the other. Thus I'm okay to use either type :) Please let me know if you want me to make the change.

This comment has been minimized.

@kmike

kmike Apr 27, 2018
Member

Fair enough, I'm fine with merging it as-is.

@kmike
Copy link
Member

@kmike kmike commented Apr 25, 2018

A good idea!

@kmike kmike changed the title Added: Allowing optional arguments for scrapy.http.cookies.CookieJar.clear [MRG+1] Added: Allowing optional arguments for scrapy.http.cookies.CookieJar.clear Apr 27, 2018
@cathalgarvey cathalgarvey changed the title [MRG+1] Added: Allowing optional arguments for scrapy.http.cookies.CookieJar.clear [MRG+2] Added: Allowing optional arguments for scrapy.http.cookies.CookieJar.clear Jun 1, 2018
@dangra dangra merged commit c6030ce into scrapy:master Jun 2, 2018
1 of 2 checks passed
1 of 2 checks passed
@codecov
codecov/patch 50% of diff hit (target 82.12%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmike kmike added this to the v1.6 milestone Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants