findcdn icon indicating copy to clipboard operation
findcdn copied to clipboard

Code cleanup

Open Pascal-0x90 opened this issue 5 years ago โ€ข 7 comments

๐Ÿ—ฃ Description

Refactoring code base to better follow python PEP-8 convention to improve future maintainability.

๐Ÿ’ญ Motivation and Context

Following issue #14, the code base should more strictly follow Python convention to hopefully improve maintainability.

๐Ÿงช Testing

Since no new functionality was added to the project, we used the same tests available in the repository to verify proper functionality. Changes to the tests are artifacts of changing how variables need to be passed to specific methods or classes in the project.

detectcdn

For this sub-module we tested all functions by running them on working and non-working domains.

  • IP resolution function
  • WHOIS function
  • CNAME function
  • HTTPS function
  • All checks function

cdnengine

For this we test if the cdnengine can properly handle sets of domains:

  • Test chef object initialization
  • Test grab_cdn with set of domains with definite CDNs
  • Test has_cdn with domains that have CDNs and domains with no CDN
  • Test that the basics of importing domains works for chef.

findcdn

We test in this module the ability for us to validate and pass information to cdnengine as well as exporting information:

  • Test basic list works to pass to cdnengine
  • Make sure our verbose setting is working
  • Test if detection of fake or broken domains works
  • Check if our input or outfiles exist or do not exist

๐Ÿšฅ Types of Changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (causes existing functionality to change)

โœ… Checklist

  • [x] My code follows the code style of this project.
  • [x] My change requires a change to the documentation.
  • [x] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [x] I have added tests to cover my changes.
  • [x] All new and existing tests passed.

Pascal-0x90 avatar Jul 31 '20 14:07 Pascal-0x90

This pull request introduces 1 alert when merging 6dce6de16c32b4f066997a518e4233e8f13e93a1 into 25eb36eed42c398995f2eb6bf55b20f9dd4008ca - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

lgtm-com[bot] avatar Jul 31 '20 14:07 lgtm-com[bot]

Fixing tests.

Pascal-0x90 avatar Aug 10 '20 17:08 Pascal-0x90

This pull request introduces 1 alert when merging a11c66708586ca15d5b6ee8b7dfcd422cce77459 into 25eb36eed42c398995f2eb6bf55b20f9dd4008ca - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

lgtm-com[bot] avatar Aug 10 '20 17:08 lgtm-com[bot]

This pull request introduces 1 alert when merging 38e77d52235c3a7f8bb942456bee1f3772b4a8ec into 25eb36eed42c398995f2eb6bf55b20f9dd4008ca - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

lgtm-com[bot] avatar Aug 10 '20 18:08 lgtm-com[bot]

@Pascal-0x90 - Thanks for fixing those two things I mentioned before, and for all the cleanup you have done here.

Ideally I'd like to see those BaseException exception blocks go away and the exec() call, but I'm not familiar enough with this code to know how to do that at the moment. Hence I'm OK with approving for now and leaving those items as future enhancements.

To make sure that this doesn't get forgotten, do you mind creating issues for those two items, mentioning the appropriate # nosec and/or #pylint comments that flag where they occur?

jsf9k avatar Aug 12 '20 01:08 jsf9k

@Pascal-0x90 - Thanks for fixing those two things I mentioned before, and for all the cleanup you have done here.

Ideally I'd like to see those BaseException exception blocks go away and the exec() call, but I'm not familiar enough with this code to know how to do that at the moment. Hence I'm OK with approving for now and leaving those items as future enhancements.

To make sure that this doesn't get forgotten, do you mind creating issues for those two items, mentioning the appropriate # nosec and/or #pylint comments that flag where they occur?

You're welcome @jsf9k . Below I have addresses some reasons for the ignores I put in the code (though really I am sure there is some alternative but I was not sure at the moment). I have opened some issues in regards to the parts of the code which include these ignores. Thank you again for your help and comments to improve the project's code ๐Ÿ˜„ .

Issues

  • Issue #18 : Use of exec() in setup.py
  • Issue #19: Use of BaseException in cdnengine.py
  • Issue #20: Use of urlopen() in cdn_check.py
  • Issue #21: Uses of ignores in __init__.py files

Pascal-0x90 avatar Aug 12 '20 14:08 Pascal-0x90

This pull request introduces 1 alert when merging 968bbb69bf91753edbbd109531d2b1c1c72f87de into 25eb36eed42c398995f2eb6bf55b20f9dd4008ca - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

lgtm-com[bot] avatar Aug 12 '20 15:08 lgtm-com[bot]