-
Notifications
You must be signed in to change notification settings - Fork 45
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
Refactoring: fix.py / linting code files / added Types #48
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,35 +11,37 @@ | |
HASH_BLOCK_SIZE = 65536 | ||
|
||
|
||
def get_filename(headers): | ||
def get_filename(headers: list) -> str: | ||
""" Retrieve a filename from a request headers via Content-Disposition | ||
""" | ||
content_disp = [x for x in headers if x[0] == 'Content-Disposition'][0][1] | ||
raw_filename = [x for x in content_disp.split(';') if x.startswith('filename=')][0] | ||
return raw_filename.replace('filename=', '').replace('"', '') | ||
|
||
|
||
def gdrive_download(gdrive_id, path): | ||
def gdrive_download(gdrive_id: str, path: str) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This functionality was inherited from upstream and it's not being used anywhere in this project. Personally, I haven't verified if it actually works either, and if possible, I'd like to remove this functionality all together. But since users might be using it in their local fixes, I guess we can't really do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have come across a number of functions that I am convinced are not being used. I left them in and updated them anyway, as it wasn't in the scope of this PR. Some stuff could be broken up, like certain functions of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this specifically, if it can be proved that the google drive functionality does not work, then that would warrant removing it. Of course, it doesn't have to be addressed in this PR. In a future separate PR, we can definitely evaluate removing some of them too. Or like, wait until the upcoming new year for the cleaning 😄 |
||
""" Download a file from gdrive given the fileid and a path to save | ||
""" | ||
url = GDRIVE_URL.format(gdrive_id) | ||
cjar = http.cookiejar.CookieJar() | ||
opener = urllib.request.build_opener(urllib.request.HTTPCookieProcessor(cjar)) | ||
urllib.request.install_opener(opener) | ||
|
||
req = urllib.request.Request(url) | ||
resp = urllib.request.urlopen(req) | ||
confirm_cookie = [x for x in resp.getheaders() if | ||
(x[0] == 'Set-Cookie' | ||
and x[1].startswith('download_warning'))][0][1] | ||
with urllib.request.urlopen(req, timeout=10) as resp: | ||
confirm_cookie = [x for x in resp.getheaders() if | ||
(x[0] == 'Set-Cookie' | ||
and x[1].startswith('download_warning'))][0][1] | ||
confirm = confirm_cookie.split(';')[0].split('=')[1] | ||
req2 = urllib.request.Request(url + '&confirm={}'.format(confirm)) | ||
resp2 = urllib.request.urlopen(req2) | ||
filename = get_filename(resp2.getheaders()) | ||
with open(os.path.join(path, filename), 'wb') as save_file: | ||
save_file.write(resp2.read()) | ||
|
||
req = urllib.request.Request(f'{url}&confirm={confirm}') | ||
with urllib.request.urlopen(req, timeout=10) as resp: | ||
filename = get_filename(resp.getheaders()) | ||
with open(os.path.join(path, filename), 'wb') as save_file: | ||
save_file.write(resp.read()) | ||
|
||
|
||
def sha1sum(filename): | ||
def sha1sum(filename: str) -> str: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same with this function. It's sorta useless and it makes more sense if the winetricks executable uses it. But unfortunately it's a shell script, and for the same above reasons, we'll probably just have to leave it |
||
""" Computes the sha1sum of the specified file | ||
""" | ||
if not os.path.isfile(filename): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm if we're adding types to non-gamefixes, type annotations should probably be enforced by the linter, Pylint, and caught in the CI system to avoid having to add the missing type in the future. This can be addressed in a separate PR though
See https://pypi.org/project/flake8-annotations/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also enforce types for the gamefixes.
This should be trivial, as most of them only consist of the
main()
function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could and it'll be keep things consistent. Though, I don't think it'll add much though so you don't need to.
For the game fixes, what we really want to catch from the linter are the typos, logical errors and some bad practices. There were just a ton of them before I added the linter in this project, and for the subset of fixes that return a non-None type for some reason I don't think it'll affect the behavior of the program.
I won't stop you from doing it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I think it is just easier to maintain one linter configuration than multiple. Alternatively there might be an option to allow functions that return
None
to have no type. Not sure about that though.