search

uniqush / uniqush-push

Uniqush is a free and open source software system which provides a unified push service for server side notification to apps on mobile devices.
http://uniqush.org
Apache License 2.0
1.54k stars 201 forks source link

Potential code quality issues found #260

Open ghost opened 4 years ago

ghost commented 4 years ago

I forked this repo a while ago and ran a DeepSource analysis on it. DeepSource found a variety of different issues categorized based on their types and severity which you can view here.

Brief description - Anti-Patterns: 36 Bug Risks: 23 Security Issues: 2

You can find a detailed description and fixes for some of them here.

Please let me know what issues you'd like to focus on fixing and I'd be happy to take a look into it. Also, you can choose to hide certain types of issues too (if you wish to ignore them or you believe it is a false positive). I'd also be happy to send a patch with the DeepSource configuration file with the required fixes.

You can find the required configuration file for DeepSource here.

TysonAndre commented 4 years ago

I've already set up golangci-lint locally and in travis, and prefer to keep using it locally and avoid multiple linter groups. Although I haven't really put much effort into configuring golangci-lint beyond the defaults or testing out the disabled linters, it seems to catch many of the same issues right now with the proper configuration https://golangci-lint.run/usage/configuration/

I'd disabled errcheck because there were too many false positives with byte buffers that shouldn't fail - I guess I could panic in that case.

The style issues seem unimportant to fix - in some cases the style was deliberate if more cases were added to an else branch or switch, though it seems unlikely for many of them.

For sha1, that's fairly low priority because tokens used by others won't be guessable and creating colliding tokens won't really matter for the ways uniqush was used


# TODO: Re-enable errcheck
linters:
    enable:
        - errcheck
        - deadcode
        - gocritic
        - gofmt
        - golint
        - gosec
        - govet
        - ineffassign
        - misspell
        - scopelint
        - structcheck
        - unconvert
        - unparam
        - varcheck
        - vet
        - vetshadow
linters-settings:
    gocritic:
        enabled-tags:
            - experimental
TysonAndre commented 4 years ago

and the Signal(os.Kill) is definitely a no-op for Kill, but I suppressed it with megacheck because there were too many issues at the time to check the docs and it would have no impact if it was a no-op.

ghost commented 4 years ago

Thank you for the inputs. Is there anything specific that you'd like me to help you out with?