Black formatting#699
Merged
Merged
Conversation
Contributor
Reviewer's Guide by SourceryThis pull request includes changes to format the code with black and remove flake8 from the GitHub Actions workflow. The primary goal is to enforce consistent code formatting and avoid conflicts between formatting tools. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @SeanTConrad - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a brief description of what each function does using docstrings.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+151
to
+153
| raise Exception( | ||
| f"{self.scanId} returned for failed report Id fetch." | ||
| ) |
Contributor
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")| ) | ||
|
|
||
| bearer_token = "Bearer "+results.stdout.decode().strip() | ||
| bearer_token = "Bearer " + results.stdout.decode().strip() |
Contributor
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use f-string instead of string concatenation [×4] (
use-fstring-for-concatenation) - Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] (
remove-redundant-slice-index)
| dry=False, | ||
| log=None | ||
| ): | ||
| def get_instances(sub_id: str, path_to_output: str = "./", dry=False, log=None): |
Contributor
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Remove unnecessary casts to int, str, float or bool (
remove-unnecessary-cast) - Use f-string instead of string concatenation (
use-fstring-for-concatenation) - Low code quality found in get_instances - 15% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
Comment on lines
433
to
438
| # protocol_separator | ||
| ps = "://" | ||
| for sp in supported_protocols: | ||
| if s.lower().startswith(sp+ps): | ||
| if s.lower().startswith(sp + ps): | ||
| return True | ||
| return False |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Use any() instead of for loop (
use-any) - Inline variable that is only used once (
inline-variable) - Use f-string instead of string concatenation (
use-fstring-for-concatenation)
Suggested change
| # protocol_separator | |
| ps = "://" | |
| for sp in supported_protocols: | |
| if s.lower().startswith(sp+ps): | |
| if s.lower().startswith(sp + ps): | |
| return True | |
| return False | |
| return any(s.lower().startswith(f"{sp}://") for sp in supported_protocols) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #699 +/- ##
=======================================
Coverage 78.00% 78.01%
=======================================
Files 31 31
Lines 2228 2229 +1
=======================================
+ Hits 1738 1739 +1
Misses 490 490 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aylusltd
reviewed
Mar 10, 2025
| assert get_url == "/report/uuid/9a6e8696-f93a-4402-a64e-342ccb37592b/CorsisCode", get_url # noqa: E501 | ||
| assert ( | ||
| agent.config.use_uuid is True | ||
| ), f"Expected True, config {agent.config}" # noqa: E501 |
Contributor
There was a problem hiding this comment.
can we remove the flake comments?
aylusltd
reviewed
Mar 10, 2025
| raise Exception("Invocation Error: must supply either a report ID or UUID") # noqa: E501 | ||
| raise Exception( | ||
| "Invocation Error: must supply either a report ID or UUID" | ||
| ) # noqa: E501 |
aylusltd
reviewed
Mar 10, 2025
| license_resp = self.getUrl(f"https://registry.npmjs.org/{entry.name}/{entry.specifier}/") # NOQA:E501 | ||
| license_resp = self.getUrl( | ||
| f"https://registry.npmjs.org/{entry.name}/{entry.specifier}/" | ||
| ) # NOQA:E501 |
aylusltd
reviewed
Mar 10, 2025
| license_resp = self.getUrl(f"{self.registrationUrl}{name}/{version}.json") # NOQA:E501 | ||
| license_resp = self.getUrl( | ||
| f"{self.registrationUrl}{name}/{version}.json" | ||
| ) # NOQA:E501 |
Contributor
There was a problem hiding this comment.
This is really repetitive. I struggled a lot to spell that. Can you tell I started this review at the bottom?
aylusltd
reviewed
Mar 10, 2025
| .content | ||
| .decode('utf-8-sig') | ||
| ) | ||
| return self.requestx.get( # noqa: E271 # NOQA: E131 |
aylusltd
reviewed
Mar 10, 2025
| MetricServiceClient, | ||
| TimeInterval, | ||
| ListTimeSeriesRequest, | ||
| ) # noqa: E501 |
Contributor
There was a problem hiding this comment.
this line is not too long.
aylusltd
reviewed
Mar 10, 2025
| findings_file_path = results_dir.joinpath('small-test-repo.git.findings.json') # NOQA: E501 | ||
| findings_file_path = results_dir.joinpath( | ||
| "small-test-repo.git.findings.json" | ||
| ) # NOQA: E501 |
aylusltd
reviewed
Mar 10, 2025
| assert ( | ||
| m | ||
| == "Data is being eval'd from a `curl` command. An attacker with control of the server in the `curl` command could inject malicious code into the `eval`, resulting in a system comrpomise. Avoid eval'ing untrusted data if you can. If you must do this, consider checking the SHA sum of the content returned by the server to verify its integrity." | ||
| ) # noqa: E501 |
aylusltd
reviewed
Mar 10, 2025
| MetricServiceClient, | ||
| TimeInterval, | ||
| ListTimeSeriesRequest, | ||
| ) # noqa: E501 |
aylusltd
reviewed
Mar 10, 2025
| Utilization_Datum as Datum, | ||
| ) | ||
|
|
||
| # os.environ["GOOGLE_APPLICATION_CREDENTIALS"] = "/Users/jason/.config/gcloud/application_default_credentials.json" # noqa: E501 |
Contributor
There was a problem hiding this comment.
it's a comment, within a comment. yodawg.gif
Contributor
There was a problem hiding this comment.
and this is a comment in response to a comment about a comment within a comment. headsplode.gif
aylusltd
reviewed
Mar 10, 2025
| "code" in self.config["modules"] and | ||
| "git" in self.config["modules"]["code"] | ||
| "repos" in self.config | ||
| and "modules" in self.config |
Contributor
There was a problem hiding this comment.
oh we gotta find a way to fix that. ands and other joiners have to be at the end of the line. This isn't just PEP wrong, it's all interpreter wrong. We should open a PR to black. (later)
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
Sean T. Conrad seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
added 2 commits
March 10, 2025 11:47
aylusltd
approved these changes
Mar 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Two changes:
I will open another PR to remove flake8 completely and make sure black runs in actions.
Related issue number
Checks
Summary by Sourcery
Apply black formatting to the codebase and remove flake8 from GitHub Actions to avoid conflicts.
Enhancements:
CI: