search

bottlepy / bottle

bottle.py is a fast and simple micro-framework for python web-applications.
http://bottlepy.org/
MIT License
8.37k stars 1.46k forks source link

Fix some code quality and bug-risk issues #1220

Closed pnijhara closed 3 years ago

pnijhara commented 4 years ago

Find the other issues found here - https://deepsource.io/gh/pnijhara/bottle/issues/?category=all

This PR also adds .deepsource.toml configuration file to run DeepSource analysis on the repo with. Upon enabling DeepSource, the analysis will run on every PR and commit to detect 560+ types of issues in the changes — including bug risks, anti-patterns, security vulnerabilities, etc.

To enable DeepSource analysis after merging this PR, please follow these steps:

  1. Signup on DeepSource with your GitHub account and grant access to this repo.
  2. Activate analysis on this repo here.

You can also look at the docs for more details. Do let me know if I can be of any help!

Marrin commented 4 years ago

Why the change from assert to if and raise AssertionError? That doesn't make sense to me.

pnijhara commented 4 years ago

Why the change from assert to if and raise AssertionError? That doesn't make sense to me.

Because while running python with -O (optimize) flag, it will remove the assert statements

Marrin commented 4 years ago

Yes I know. This is expected from assert statements. assert is for testing things you know are are true. As a safety net for really surprising edge cases or bugs, which can be turned off when you need/want the speed. This is by design, so I still don't understand the change.

If you think that shouldn't be an assertion but regular, proper, explicit error checking, then AssertionError is wrong. That would be a TypeError instead. IMHO.

pnijhara commented 4 years ago

@Marrin I agree with your suggestions. I am replacing AssertionError with TypeError.

pnijhara commented 4 years ago

@Marrin Please review this again!

defnull commented 4 years ago

The current CI stack does not test with deepsource, so there is no real benefit in adding a configuration file or even bundling it with the distribution. Does this file really need to be part of the source distribution?

The changes are also mostly cosmetic. There is no real bug-risk there. The list -> generator change may also impact performance. There was a time where str.join() was significantly faster with a list for short lists. I am unsure if that is still the case.

I never used deepsource. Does it really only detect these two issues?

pnijhara commented 4 years ago

The current CI stack does not test with deepsource, so there is no real benefit in adding a configuration file or even bundling it with the distribution. Does this file really need to be part of the source distribution?

The analysis configuration for a repository on DeepSource is defined in a .deepsource.toml file in the repository's root. This file must be present at the said location for analysis to be run. Moreover, this file does not require any tests and will not hinder the distribution.

The changes are also mostly cosmetic. There is no real bug-risk there. The list -> generator change may also impact performance. There was a time where str.join() was significantly faster with a list for shortlists. I am unsure if that is still the case. I never used deepsource. Does it really only detect these two issues?

@defnull, DeepSource config ensures issues like these don't make it to master, primarily from external contributors. The Python analyzer detects over 520 types of issues categorized as bug-risks, anti-patterns, performance and security. The tool is free to use for open-source and is used by projects like https://github.com/uber/ludwig and https://github.com/slackapi/python-slackclient/

You can look at the other issues found here - https://deepsource.io/gh/pnijhara/bottle/issues/?category=all

RonRothman commented 3 years ago

DeepSource config ensures issues like these don't make it to master

No, it does nothing if the repo owner doesn't go through the trouble of creating an account and granting access.

I looked over the issues that DeepSource found in the Bottle repository. They're all minor, and almost every single one of them would be caught by a tool like pylint, which, unlike DeepSource, is an industry standard and requires no new accounts to be created or permissions to be granted.

If you want to address the minor issues that DeepSource found, that would be great (and thank you!). But there's no need to clutter Bottle's streamlined codebase with config files for obscure services that will never be used.

pnijhara commented 3 years ago

DeepSource config ensures issues like these don't make it to master

No, it does nothing if the repo owner doesn't go through the trouble of creating an account and granting access.

I looked over the issues that DeepSource found in the Bottle repository. They're all minor, and almost every single one of them would be caught by a tool like pylint, which, unlike DeepSource, is an industry standard and requires no new accounts to be created or permissions to be granted.

If you want to address the minor issues that DeepSource found, that would be great (and thank you!). But there's no need to clutter Bottle's streamlined codebase with config files for obscure services that will never be used.

Agreed with your statement. But DeepSource is completely different from pylint. Moreover, DeepSource is working in the config-free working of the tool.

defnull commented 3 years ago

DeepSource wants to "Act on my behalf" when I try to log-in with my github account. No. Just no.