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

Code cleanup #16

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Code cleanup #16

wants to merge 8 commits into from

Conversation

Pascal-0x90
Copy link
Collaborator

@Pascal-0x90 Pascal-0x90 commented Jul 31, 2020

🗣 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

  • 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

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

mcdonnnj and others added 4 commits July 14, 2020 12:34
Add pylint as a testing requirement in setup.py, add a local hook using it to
pre-commit-config.yaml, and add a slightly tweaked .pylintrc file for
configuration.
Signed-off-by: Pascal-0x90 <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Jul 31, 2020

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

new alerts:

  • 1 for Except block handles 'BaseException'

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few questions about this PR, which may or may not require changes. Please see my comments.

src/findcdn/cdnengine/cdnengine.py Outdated Show resolved Hide resolved
src/findcdn/findcdn.py Outdated Show resolved Hide resolved
@Pascal-0x90
Copy link
Collaborator Author

Fixing tests.

@lgtm-com
Copy link

lgtm-com bot commented Aug 10, 2020

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

new alerts:

  • 1 for Except block handles 'BaseException'

Signed-off-by: Pascal-0x90 <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Aug 10, 2020

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

new alerts:

  • 1 for Except block handles 'BaseException'

@Pascal-0x90 Pascal-0x90 requested a review from jsf9k August 11, 2020 18:23
@jsf9k
Copy link
Member

jsf9k commented Aug 12, 2020

@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?

@Pascal-0x90
Copy link
Collaborator Author

@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

Removed comment about timeout and explained use of \#nosec
Copy link
Collaborator Author

@Pascal-0x90 Pascal-0x90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed comment in file to reference use of nosec instead of timeout which has no default.

src/findcdn/cdnengine/detectcdn/cdn_check.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Aug 12, 2020

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

new alerts:

  • 1 for Except block handles 'BaseException'

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.

4 participants