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

Windows fixes #27

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Windows fixes #27

wants to merge 6 commits into from

Conversation

laerreal
Copy link

@laerreal laerreal commented Jan 6, 2019

Hello! Ricently I used this tool under Windows 7 x64, Python 2.7.15 x86_64. There are few fixes I made.

Using OS specific separator instead of : will cause misses on old caches. But this does make new caches portable.

@sebinthomas
Copy link

Hi @laerreal

Welcome to Cappy and thank you for the PR. I've gone through it and it looks good to merge except for some small issues.

hash_template = '{method}%s{stub}{param_str}' % os.sep

This would add an extra slash to hash_template and as a result, create an extra directory as a side-effect. hash_template would be something like GET/index.html instead of GET:index.html and make_dirs function would create an extra directory called GET.

While this works without any issues, I'd rather have this documented somewhere than have this as a side-effect. Maybe we could use _ or - as a separator between {method} and {stub}

@cyriac what do you think ?

@cyriac
Copy link
Member

cyriac commented Jan 7, 2019

Hi @laerreal. Thanks for the contribution.

As @sebinthomas suggested, this would cause one additional directory to be created that is not necessarily adding value. Could we use a different separator there?

Also instead of using os.sep.join can we use os.path.join to keep it consistent?

@laerreal
Copy link
Author

laerreal commented Jan 7, 2019

Hi. Using slash as seperator also preserves original file names. This can be convenient under some circumstances.
I also noticed yet another problem. It's with using of ? for param string. That symbol is also forbidden under Windows.
Also, suffxing anything after file extension will confuse Windows about file type.
Of course, a NIX OS is robust against those problems.

So, I suggest next:

  1. use (space) as a separator for both method and its parameters.
  2. place parameters between method and stub:
'{method} {param_str} {stub}'

Also, is it possible in HTTP request, a raw space is present in either file name or parametrs? If not, this approch will also revent possible confusion with requests of files with names like "GET-myfile.html".

What do you think?

P.S. I will replace os.sep.join with os.path.join.

@cyriac
Copy link
Member

cyriac commented Jan 8, 2019

How about we convert get_hashed_filepath to return an md5 or sha1 hash of the resultant string? This would avoid all of the problems.

So the code would roughly translate to something like this

def get_hashed_filepath(stub, method, parsed_url, params):
    hash_template = '{method}%s{stub}{param_str}' % os.sep  # Formatting for this can be anything

    ... other code ....

    filepath = hash_template.format(method=method, stub=stub, param_str=param_str)
    return hashlib.md5(filepath).hexdigest()

@laerreal
Copy link
Author

laerreal commented Jan 9, 2019

This approach results in hiding of file names.

default

I think, at least, the hash have to be used as a directory name. But it's appearance issue only.

@sebinthomas
Copy link

@laerreal I suggest you keep

hash_template = '{method}_{stub}{param_str}'
hash_str  = hash_template.format(method=method, stub=stub, param_str=param_str)

and do

return md5(hash_str).hexdigest()

This makes it easy if you want to revert or see the filename/stub if you're debugging. Notice the _ after {method} instead of os.sep

Looks good to merge otherwise.

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.

3 participants