Skip to content
This repository has been archived by the owner on Aug 28, 2020. It is now read-only.

refactor code #107 #161

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

themousepotato
Copy link

@themousepotato themousepotato commented Oct 26, 2018

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

If you have questions, please send an email to SendGrid, or file a GitHub Issue in this repository.

@SendGridDX
Copy link

SendGridDX commented Oct 26, 2018

CLA assistant check
All committers have signed the CLA.

@themousepotato
Copy link
Author

@thinkingserious Why isn't the build successful? How can I set the environment variables? I haven't found any documentation for this.

Copy link

@42B 42B left a comment

Choose a reason for hiding this comment

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

This PR doesn't make sense and the changes seem superfluous at best. These chained if statements do not need to be broken up into two nested if statements each. Python already short-circuits and will not even evaluate the second check if the first one fails in cases like this using and.

@themousepotato
Copy link
Author

themousepotato commented Oct 28, 2018 via email

@42B
Copy link

42B commented Oct 28, 2018

@themousepotato

I'm not from sendgrid so I would defer to them as to whether they find this PR useful or not. I'm having trouble seeing how this PR would reduce complexity mentioned in #107 or add any real benefit to the repo but those decisions are not up to me. All you did was break chained if statements into two separate if statements each but you didn't reduce the total number of if statements at all?

Maybe you could give a detailed description on exactly how this PR reduces the complexity score from codeclimate? After that, if the previous score was 27 and the maximum should be 5, what is the new complexity score after your changes were made?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix "method_complexity" issue in package_managers.py
4 participants