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

support for ignoring get params #20 #22

Merged
merged 5 commits into from Jul 25, 2017

Conversation

Projects
None yet
3 participants
@tony
Copy link
Collaborator

tony commented Jul 25, 2017

Also, decouple off yes/no functionality into yesno_to_bool function.

support for ignoring get params #20
Also, decouple off yes/no functionality into ``yesno_to_bool`` function.
@hellysmile
Copy link
Owner

hellysmile left a comment

Add README.rst notes please

# check missing href parameter
if not url.attrib.get('href', None) is None:
# get href attribute
href = url.attrib['href'].strip()
# cut off hashtag (anchor)
href = re.sub(r'\#.+', '', href)
if ignore_params:

This comment has been minimized.

@hellysmile

hellysmile Jul 25, 2017

Owner

use urlparse.urlunsplit urlparse.urlunsplit here please

This comment has been minimized.

@tony

tony Jul 25, 2017

Collaborator

This is actually less performant IMO, but I added it in 21ce8a3. While it feels good to work with the standard library.

  1. it turns out urlparse objects aren't mutable. The reason why is the object subclasses tuple.
  2. _replace is available, but still does a copy every time

So don't you think re.sub would be more performant? Since there's no additional urlsplit / tuple object created?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 25, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.315% when pulling b0b0242 on develtech:ignore_params into dc0c979 on hellysmile:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling b758829 on develtech:ignore_params into dc0c979 on hellysmile:master.

@hellysmile hellysmile merged commit 41b25cc into hellysmile:master Jul 25, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment