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

Remove or Explain Manual Testing Check Disables #60

Merged
merged 4 commits into from
Nov 18, 2020

Conversation

mcdonnnj
Copy link
Member

@mcdonnnj mcdonnnj commented Nov 16, 2020

🗣 Description

This PR changes the code in setup.py used to get version information and adds a comment explaining why a Flake8 check is disabled. This closes #58 .

💭 Motivation and Context

This project had two check disabling comments with no explanation: one each for Bandit and Flake8. The lines of code in question needed to either be changed to remove the need for disabling a check, or to have a comment explaining why the check disable was required. I felt it was better to refactor the code to remove the need for the Bandit check to be disabled. However the piece of code that needs a Flake8 check disabled is the most straightforward way to handle how we implement a Single Source of Truth for package versioning.

🧪 Testing

Automated tests pass.

✅ Checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

Switch to the file line reading version of extracting the version from
https://packaging.python.org/guides/single-sourcing-package-version/
instead of the exec method on the same page. The exec method required us to use
a "# nosec" to manually disable Bandit checking on that line. Although that
method is more straightforward, I do not feel that it is worth using an exec in
the codebase when another option is available.
@lgtm-com
Copy link

lgtm-com bot commented Nov 16, 2020

This pull request introduces 1 alert when merging 40b3b80 into 0f6efe0 - view on LGTM.com

new alerts:

  • 1 for Unnecessary 'else' clause in loop

@mcdonnnj mcdonnnj changed the title Change or Add Explanatory Comments to Manual Check Disables Remove or Explain Manual Testing Check Disables Nov 16, 2020
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.

👍

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Nice improvements! 👍 👍

Copy link
Contributor

@hillaryj hillaryj left a comment

Choose a reason for hiding this comment

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

🥇

cisagovbot pushed a commit that referenced this pull request Nov 18, 2020
@mcdonnnj mcdonnnj merged commit b32bb9a into develop Nov 18, 2020
@mcdonnnj mcdonnnj deleted the resolve_manual_check_disables branch November 18, 2020 21:01
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.

Explain uses of noqa and nosec in codebase
4 participants