Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add bugbear, add B00 to ignored checks, and fix 2 violation codes #9499

Merged
merged 3 commits into from
Mar 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/9499.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Introduce bugbear to the test suite and fix some of it's lint violations.
3 changes: 2 additions & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ ignore =
# E203: whitespace before ':' (which is contrary to pep8?)
# E731: do not assign a lambda expression, use a def
# E501: Line too long (black enforces this for us)
ignore=W503,W504,E203,E731,E501
# B00: Subsection of the bugbear suite (TODO: add in remaining fixes)
ignore=W503,W504,E203,E731,E501,B00

[isort]
line_length = 88
Expand Down
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ def exec_file(path_segments):
"isort==5.7.0",
"black==20.8b1",
"flake8-comprehensions",
"flake8-bugbear",
"flake8",
]

Expand Down
4 changes: 3 additions & 1 deletion synapse/app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
try:
python_dependencies.check_requirements()
except python_dependencies.DependencyException as e:
sys.stderr.writelines(e.message)
sys.stderr.writelines(
e.message # noqa: B306, DependencyException.message is a property
Copy link
Member

Choose a reason for hiding this comment

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

Should we just be using str(e) in each of these spots as the bugbear docs suggest?

Copy link
Contributor Author

@ShadowJonathan ShadowJonathan Mar 3, 2021

Choose a reason for hiding this comment

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

no, because DependencyException.message is a custom property (to DependencyException) that returns a string value. It was probably done that way because in python 2.7, BaseException.message was used to signal the actual 'message', it's deprecated in python 3, and thats why bugbear is complaining, i'm explicitly suppressing this here because it is technically still valid, but i'm also putting after the noqa why it is surpressed. (with saying that the .message property under DependencyException specifically is a valid property which bugbear cannot know or be pleased by)

If we'd wanna have it work with str(e), we need to override __str__ on DependencyException

Copy link
Member

Choose a reason for hiding this comment

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

This seems to be PyCQA/flake8-bugbear#131.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm, thanks for that mention, ive subbed to it.

But in the meantime, would this be accepted? (noqa on this, with the reasoning behind it as well)

)
sys.exit(1)


Expand Down
6 changes: 5 additions & 1 deletion synapse/config/key.py
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,11 @@ def _parse_key_servers(key_servers, federation_verify_certificates):
try:
jsonschema.validate(key_servers, TRUSTED_KEY_SERVERS_SCHEMA)
except jsonschema.ValidationError as e:
raise ConfigError("Unable to parse 'trusted_key_servers': " + e.message)
raise ConfigError(
"Unable to parse 'trusted_key_servers': {}".format(
e.message # noqa: B306, jsonschema.ValidationError.message is a valid attribute
)
)

for server in key_servers:
server_name = server["server_name"]
Expand Down
4 changes: 3 additions & 1 deletion synapse/config/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ def read_config(self, config, **kwargs):
try:
check_requirements("sentry")
except DependencyException as e:
raise ConfigError(e.message)
raise ConfigError(
e.message # noqa: B306, DependencyException.message is a property
)

self.sentry_dsn = config["sentry"].get("dsn")
if not self.sentry_dsn:
Expand Down
4 changes: 3 additions & 1 deletion synapse/config/oidc_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ def read_config(self, config, **kwargs):
try:
check_requirements("oidc")
except DependencyException as e:
raise ConfigError(e.message) from e
raise ConfigError(
e.message # noqa: B306, DependencyException.message is a property
) from e

# check we don't have any duplicate idp_ids now. (The SSO handler will also
# check for duplicates when the REST listeners get registered, but that happens
Expand Down
4 changes: 3 additions & 1 deletion synapse/config/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,9 @@ def read_config(self, config, **kwargs):
check_requirements("url_preview")

except DependencyException as e:
raise ConfigError(e.message)
raise ConfigError(
e.message # noqa: B306, DependencyException.message is a property
)

if "url_preview_ip_range_blacklist" not in config:
raise ConfigError(
Expand Down
4 changes: 3 additions & 1 deletion synapse/config/saml2_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ def read_config(self, config, **kwargs):
try:
check_requirements("saml2")
except DependencyException as e:
raise ConfigError(e.message)
raise ConfigError(
e.message # noqa: B306, DependencyException.message is a property
)

self.saml2_enabled = True

Expand Down
4 changes: 3 additions & 1 deletion synapse/config/tracer.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ def read_config(self, config, **kwargs):
try:
check_requirements("opentracing")
except DependencyException as e:
raise ConfigError(e.message)
raise ConfigError(
e.message # noqa: B306, DependencyException.message is a property
)

# The tracer is enabled so sanitize the config

Expand Down
2 changes: 1 addition & 1 deletion synapse/crypto/context_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ def clientConnectionForTLS(self, tls_protocol):
# ... and we also gut-wrench a '_synapse_tls_verifier' attribute into the
# tls_protocol so that the SSL context's info callback has something to
# call to do the cert verification.
setattr(tls_protocol, "_synapse_tls_verifier", self._verifier)
tls_protocol._synapse_tls_verifier = self._verifier
return connection


Expand Down
2 changes: 1 addition & 1 deletion tests/unittest.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ def assertObjectHasAttributes(self, attrs, obj):
try:
self.assertEquals(attrs[key], getattr(obj, key))
except AssertionError as e:
raise (type(e))(e.message + " for '.%s'" % key)
raise (type(e))("Assert error for '.{}':".format(key)) from e
clokep marked this conversation as resolved.
Show resolved Hide resolved

def assert_dict(self, required, actual):
"""Does a partial assert of a dict.
Expand Down