Handle invalid domains more gracefully#53
Conversation
|
This pull request introduces 1 alert when merging b3b320d into 7bbf50e - view on LGTM.com new alerts:
Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog. |
Pascal-0x90
left a comment
There was a problem hiding this comment.
I like the changes. I think this is a great addition. Going to say this should be merged on the note that I am also going to be working more of the PRs too so this may also end up being rearranged. I want to get rid of the complexity you keep pointing out because I am also noticing that as well.
Pascal-0x90
left a comment
There was a problem hiding this comment.
One thing I forgot to mention, please make sure you run pre-commit hooks before merging so the tests pass. Looks like pre-commit hooks had some issues.
|
I think those "test_list_broken/est_file_broken" tests that fail, were failing before. I actually don't even understand the tests to be honest. I'll double check with the other branches. |
|
Good point. The tests should probably get a good review as well while I am on this. |
🗣 Description
As per #42, invalid domains should be handled gracefully.
These are checked for in main() before running the executor, and put in a separate "invalid domains" bucket.
Also, as part of code cleanup, the domain class was moved to a separate python file.
There is a LOT to do and a lot of redundant call chains to be removed, but each journey starts with a single step!
💭 Motivation and context
🧪 Testing
Added unit tests for valid/invalid cdnCheck() and main() calls with valid/invalid domains
Sample output
✅ Pre-approval checklist
in code comments.
to reflect the changes in this PR.
✅ Pre-merge checklist
✅ Post-merge checklist