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

v0.6.2 #33

Merged
merged 10 commits into from
Nov 4, 2021
Merged

v0.6.2 #33

merged 10 commits into from
Nov 4, 2021

Conversation

stolarczyk
Copy link
Member

fix #32

@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #33 (abe2ef3) into master (771ffbf) will increase coverage by 0.78%.
The diff coverage is 72.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   85.58%   86.36%   +0.78%     
==========================================
  Files          18       19       +1     
  Lines         853      851       -2     
==========================================
+ Hits          730      735       +5     
+ Misses        123      116       -7     
Impacted Files Coverage Δ
setup.py 0.00% <0.00%> (ø)
ubiquerg/collection.py 56.89% <50.00%> (ø)
ubiquerg/cli_tools.py 82.73% <68.75%> (ø)
ubiquerg/files.py 75.40% <80.00%> (ø)
tests/test_web.py 100.00% <100.00%> (ø)
ubiquerg/_version.py 100.00% <100.00%> (ø)
ubiquerg/environment.py 100.00% <100.00%> (ø)
ubiquerg/paths.py 69.56% <100.00%> (-1.27%) ⬇️
ubiquerg/web.py 100.00% <100.00%> (+22.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 771ffbf...abe2ef3. Read the comment docs.

@nsheff nsheff requested a review from vreuter January 28, 2021 15:30
@nsheff
Copy link
Contributor

nsheff commented Jan 28, 2021

this switches to a regex-based approach. this will be much slower, but will probably be more what we're looking for... @vreuter any objections?

@stolarczyk
Copy link
Member Author

stolarczyk commented Jan 28, 2021

That's an overkill for such insignificant problem... but ran couple of tests and you're right, regex is slower since it depends on the input length (O(n) time). The urlparse approach runs in O(1) time

import time
from urllib.parse import urlparse 
from ubiquerg import is_url

def check_time(fun, n=10000):
    results = []
    for _ in range(1, n):
        start = time.time()
        fun(string)
        end = time.time() 
        results.append((end - start))
    return (sum(results) / n) * 1000


def is_url_old(string): 
    try:
        result = urlparse(string)
        return all([result.scheme, result.netloc, result.path])
    except:
        return False

strings = [f"http://www.g{'o' * 10000}gle.com'", f"http://www.g{'o' * 5000}gle.com'", "http://www.google.com", ""]
for string in strings:
    print(f"new time: {check_time(fun=is_url)}ms")
    print(f"old time: {check_time(fun=is_url_old)}ms\n")
~ python3 ~/Desktop/check_time.py 
new time: 1.1539672136306762ms
old time: 0.003525686264038086ms

new time: 0.6307248592376709ms
old time: 0.003618669509887695ms

new time: 0.004984068870544433ms
old time: 0.0035085678100585938ms

new time: 0.0031899452209472657ms
old time: 0.0036333560943603515ms

tests/test_web.py Outdated Show resolved Hide resolved
return urlparse(maybe_url).scheme != ""
# from Django 1.3.x
# https://github.com/django/django/blob/6726d750979a7c29e0dd866b4ea367eef7c8a420/django/core/validators.py#L45-L51
regex = re.compile(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

Copy link
Member

@vreuter vreuter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave an optional suggestion as a comment but looks good, w/e you guys think as far as tradeoff b/w runtime vs. behavior of the implementation.

@nsheff nsheff merged commit 1d79b7c into master Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make is_url more strict?
3 participants