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

Fix error if regex can't find the filename #3285

Merged

Conversation

davidculley
Copy link
Contributor

Ensured that the returned match object is not None.

@abdnh
Copy link
Collaborator

abdnh commented Jul 11, 2024

Doesn't this just make the error less descriptive? Assuming this is ever hit by users.

@davidculley
Copy link
Contributor Author

Fixed it.

Now it prints an error message more helpful than a non-descriptive 'NoneType' object has no attribute 'group' if the string didn't match the regular expression. This also makes the type checker happy.

It also no longer uses exceptions to control the flow.

@davidculley davidculley force-pushed the feature/treat-case-if-filename-not-found branch from 980a746 to b374bea Compare July 11, 2024 16:16
@dae
Copy link
Member

dae commented Jul 21, 2024

Is this correcting an error you were seeing in the wild? I would not expect AnkiWeb to be be sending a 200 return code if the header is missing.

@davidculley
Copy link
Contributor Author

davidculley commented Jul 27, 2024

No, this is fixing a type error reported by Pyright.

Since I use type checkers and linters quite extensively, I'd like them to be happy with the code so that the critical problems can't go unnoticed.

If the code is overengineered, I'd like to see this instead (replaced the if-block with an assertion):

match = re.match(
    "attachment; filename=(.+)", resp.headers["content-disposition"]
)
assert match is not None
fname = match.group(1)

@dae
Copy link
Member

dae commented Aug 4, 2024

Happy to see typechecking issues fixed. When we expect something never to be None, I feel an assert better conveys that than an if statement, and it adds less noise to the codebase.

@davidculley
Copy link
Contributor Author

an assert better conveys that

Done.

@dae
Copy link
Member

dae commented Aug 7, 2024

Thanks!

@dae dae merged commit 70a063a into ankitects:main Aug 7, 2024
1 check passed
@davidculley davidculley deleted the feature/treat-case-if-filename-not-found branch August 10, 2024 03:47
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