-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Remove unnecessary blank (_) identifier #551
Conversation
.deepsource.toml
Outdated
@@ -0,0 +1,10 @@ | |||
version = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a local file which shouldn't be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a configuration file required to activate analysis in the repo. Apart from finding issues in Go, You can use DeepSource to track test coverage, detect problems in Dockerfiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenh any update on this? 💖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wricardo sorry I missed your reply there.
I'm happy merge the code fix but would need to do some more research on deepsource as looks like its triggering some false positives in that link.
For other projects I've used golangci-lint which is good so will have a look at adding that here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Draft of the golangci-lint is here: #554 if you're interested in helping review as there are lots of fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5% is pretty high IMO, lets get the fix merged without deepsource then we can look at that later, without blocking the fix.
@stevenh I have removed the configuration file. I would really appreciate it if you could drop a suggestion on what we can do onboard you(or redigo) on DeepSource in the future. 💖 |
@stevenh 👋 just want to follow up on this! |
Thank you for the PR, new release was cut last night too, which includes a bunch more fixes :) |
Can I submit a new PR with the configuration file which I used to configure the analysis? |
I believe all of those are already fixed. If there are issues which DeepSource detects that aren't I'd like to hear about that. |
I updated my forked master and ran the analysis. DeepSource caught security issues that you might want to fix. |
Thanks @withshubh. I had a quick look, the sha1 errors are false positives as its not being used for crypto and the tls one is a fallback when the user doesn't provide a config, hence needs to be permissive for usability. It could be argued that this should just return an error but that wouldn't be backwards compatible :( |
Got it. |
Hi 👋
I ran the DeepSource static analyzer on the forked copy of this repo and found some interesting code quality issues. This PR fixed a few of them.
Summary of changes
Added .deepsource.toml