Skip to content

misc: [DPE-7961] new linting rules #286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

misc: [DPE-7961] new linting rules #286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion poetry.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

54 changes: 39 additions & 15 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,20 @@ opentelemetry-exporter-otlp-proto-http = "1.21.0"
optional = true

[tool.poetry.group.format.dependencies]
ruff = "^0.12.4"
ruff = "^0.12.7"

[tool.poetry.group.lint]
optional = true

[tool.poetry.group.lint.dependencies]
ruff = "^0.12.4"
ruff = "^0.12.7"
codespell = "^2.4.1"

[tool.poetry.group.unit.dependencies]
pytest = "^8.4.1"
pytest-xdist = "^3.8.0"
pytest-cov = "^6.2.1"
ops-scenario = "^6.0.3, <6.0.4" # 6.0.4 requires ops >= 2.12
ops-scenario = "^6.0.3, <6.0.4" # 6.0.4 requires ops >= 2.12

[tool.poetry.group.integration.dependencies]
pytest = "^8.4.1"
Expand Down Expand Up @@ -84,23 +84,47 @@ line-length = 99

[tool.ruff.lint]
explicit-preview-rules = true
select = ["A", "E", "W", "F", "C", "N", "D", "I", "CPY001"]
select = [
"A",
"E",
"W",
"F",
"C",
"N",
"D",
"I",
"B",
"CPY001",
"RUF",
"S",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"S",

looks like this causes a lot of false positives, don't think it's worth the hassle
e.g. https://github.com/search?q=repo%3Acanonical%2Fmysql-operator%20s105&type=code

Copy link

Choose a reason for hiding this comment

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

👋🏻 My 2 cents about this topic is that the S linting rule should stay, as there are a bunch of security related misconfigurations it could detect. I recognize S105 is annoying, but I think the benefits outweigh the drawbacks.

"SIM",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"SIM",

imo, many of these rules decrease readability of the code

Copy link

Choose a reason for hiding this comment

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

👋🏻 My 2 cents about this topic is that the SIM linting rule should not stay, as some of the rules enforce syntax that we may not want to apply, either for readability purposes, or because we want to sneak a comment right in the middle of two statements.

It it up to you if you want to fine-tune the rules and select only those that are overwhelmingly benign (i.e. double-negation, expr-with-true-false...)

"UP",
"TC",
]
ignore = [
# Missing docstring in public method (pydocstyle doesn't look for docstrings in super class
# https://github.com/PyCQA/pydocstyle/issues/309) TODO: add pylint check? https://github.com/PyCQA/pydocstyle/issues/309#issuecomment-1284142716
"D102",
"D105", # Missing docstring in magic method
"D107", # Missing docstring in __init__
"D403", # First word of the first line should be capitalized (false positive on "MySQL")
"D415", # Docstring first line punctuation (doesn't make sense for properties)
"E501", # Line too long (because using black creates errors with this)
"N818", # Exception name should be named with an Error suffix
"W505", # Doc line too long (so that strings in comments aren't split across lines)
# Missing docstring in public method (pydocstyle doesn't look for docstrings in super class
# https://github.com/PyCQA/pydocstyle/issues/309) TODO: add pylint check? https://github.com/PyCQA/pydocstyle/issues/309#issuecomment-1284142716
"D102",
"D105", # Missing docstring in magic method
"D107", # Missing docstring in __init__
"D403", # First word of the first line should be capitalized (false positive on "MySQL")
"D415", # Docstring first line punctuation (doesn't make sense for properties)
"E501", # Line too long (because using black creates errors with this)
"N818", # Exception name should be named with an Error suffix
"W505", # Doc line too long (so that strings in comments aren't split across lines)
"S101",
Copy link
Author

Choose a reason for hiding this comment

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

S101 is the only differing config between this and others (mysql-server, pg*), and it's related to the usage of assert's in the code.
@carlcsaposs-canonical I would prefer removing those, but I've added the ignore to get your position.

Copy link
Contributor

Choose a reason for hiding this comment

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

charms aren't run with -O, so asserts should always run

I think asserts are valuable for

  • documenting assumptions that should never get raised
  • they are much shorter than raising exceptions

I think adding s101 will result in less assumptions getting documented (an assert is much cheaper than if: raise)

yes we should raise exceptions (and not assert) for expected errors, but I don't think we should get rid of assert for documenting assumptions

ctrl-f "assert" in https://github.com/canonical/charm-refresh/blob/main/charm_refresh/_main.py for some examples of things I wouldn't bother writing code to raise an exception for but is worth writing an assert to document the assumption

Copy link
Author

Choose a reason for hiding this comment

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

My 2 cents is that asserts in production code is weird, due:

they are much shorter than raising exceptions

Short code is not something to pursue and I disregard it as a good argument. We should pursue being explicit instead.

documenting assumptions that should never get raised

Being explicit does a better job and, assert's will result in lost of granularity for error catching as all errors will be AssertionError

Copy link
Contributor

@carlcsaposs-canonical carlcsaposs-canonical Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

assert's will result in lost of granularity for error catching

imo we should never be catching asserts—if we're catching, a proper exception should be raised

Short code is not something to pursue

I think it's significantly more readable and, practically speaking, the alternative would be to not document assumptions at all instead of raising exceptions
for a concrete example, I think the refresh code linked above would be a lot harder to follow if the asserts were replaced with if: raise—assert makes it clear that it's validating an assumption instead of an expected error case and it keeps the "business logic" clear/front and center

Copy link

Choose a reason for hiding this comment

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

👋🏻 My 2 cents about this topic is that we should avoid using assertions.

I agree that the assertions are shorten than properly defined + raised exceptions, but I think we should strive for the latter and do not take shortcuts. Both the name and doc-string of the substitute exceptions will replace the documentation value of the assertions, possibly even surpassing it.

In short: I am in favor of applying S101.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"S101",
"S101",
"B011",
"B903",
"B904", # Exception chaining happens automatically if `from` omitted
"RUF005",
"RUF010",

b011 for same reason as s101
b903 class with only __init__ shouldn't always be a dataclass
b904 unnecessarily verbose, default behavior of exception chaining (when from omitted) is almost always desired
ruf005 imo premature optimization; sometimes less readable
ruf010 imo decreases readability

Copy link

Choose a reason for hiding this comment

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

👋🏻 My 2 cents here highly varies from rule to rule:

  • B011 - Unnecessary if S101 gets apply, which I am in favour of.
  • B903 - No preference.
  • B904 - No preference, although if exception names are fine-grained enough, I will say unnecessay.
  • RUF005 - I like it, and find it more readable than concatenating using +.
  • RUF010 - No preference.

]

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[tool.ruff.lint.ruff]
parenthesize-tuple-in-subscript = true

change behavior of ruf031 to be more readable

[tool.ruff.lint.per-file-ignores]
# D100, D101, D102, D103: Ignore missing docstrings in tests
"tests/*" = ["D1"]
"tests/*" = [
"D1",
"D417",
# Asserts
"B011",
# Disable security checks for tests
"S",
]

[tool.ruff.lint.flake8-copyright]
# Check for properly formatted copyright header in each file
Expand Down
5 changes: 2 additions & 3 deletions src/abstract_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,8 @@ def _determine_unit_status(self, *, event) -> ops.StatusBase:
if status := workload_.status:
statuses.append(status)
# only in machine charms
if self._ha_cluster:
if status := self._ha_cluster.get_unit_juju_status():
statuses.append(status)
if self._ha_cluster and (status := self._ha_cluster.get_unit_juju_status()):
statuses.append(status)
refresh_lower_priority = self.refresh.unit_status_lower_priority(
workload_is_running=isinstance(workload_, workload.RunningWorkload)
)
Expand Down
22 changes: 13 additions & 9 deletions src/container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ def mysql_router_exporter_service_enabled(self) -> bool:
"""MySQL Router exporter service status"""

@abc.abstractmethod
def update_mysql_router_service(self, *, enabled: bool, tls: bool = None) -> None:
def update_mysql_router_service(
self, *, enabled: bool, tls: typing.Optional[bool] = None
) -> None:
"""Update and restart MySQL Router service.

Args:
Expand All @@ -148,10 +150,10 @@ def update_mysql_router_exporter_service(
*,
enabled: bool,
config: "relations.cos.ExporterConfig" = None,
tls: bool = None,
key_filename: str = None,
certificate_filename: str = None,
certificate_authority_filename: str = None,
tls: typing.Optional[bool] = None,
key_filename: typing.Optional[str] = None,
certificate_filename: typing.Optional[str] = None,
certificate_authority_filename: typing.Optional[str] = None,
) -> None:
"""Update and restart the MySQL Router exporter service.

Expand Down Expand Up @@ -207,7 +209,7 @@ def _run_command(
command: typing.List[str],
*,
timeout: typing.Optional[int],
input: str = None, # noqa: A002 Match subprocess.run()
input: typing.Optional[str] = None, # noqa: A002 Match subprocess.run()
) -> str:
"""Run command in container.

Expand All @@ -216,7 +218,9 @@ def _run_command(
"""

# TODO python3.10 min version: Use `list` instead of `typing.List`
def run_mysql_router(self, args: typing.List[str], *, timeout: int = None) -> str:
def run_mysql_router(
self, args: typing.List[str], *, timeout: typing.Optional[int] = None
) -> str:
"""Run MySQL Router command.

Raises:
Expand All @@ -230,8 +234,8 @@ def run_mysql_shell(
self,
args: typing.List[str],
*,
timeout: int = None,
input: str = None, # noqa: A002 Match subprocess.run()
timeout: typing.Optional[int] = None,
input: typing.Optional[str] = None, # noqa: A002 Match subprocess.run()
) -> str:
"""Run MySQL Shell command.

Expand Down
2 changes: 1 addition & 1 deletion src/machine_workload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def _get_bootstrap_command(
if self._charm.is_externally_accessible(event=event):
command.extend([
"--conf-bind-address",
"0.0.0.0",
"0.0.0.0", # noqa: S104
])
else:
command.extend([
Expand Down
14 changes: 7 additions & 7 deletions src/mysql_shell/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ def username(self):
def _run_code(self, code: str) -> None:
"""Connect to MySQL cluster and run Python code."""
template = _jinja_env.get_template("try_except_wrapper.py.jinja")
error_file = self._container.path("/tmp/mysqlsh_error.json")
error_file = self._container.path("/tmp/mysqlsh_error.json") # noqa: S108

script = template.render(code=code, error_filepath=error_file.relative_to_container)

temporary_script_file = self._container.path("/tmp/mysqlsh_script.py")
temporary_script_file = self._container.path("/tmp/mysqlsh_script.py") # noqa: S108
temporary_script_file.write_text(script)

try:
Expand Down Expand Up @@ -100,7 +100,7 @@ def _run_code(self, code: str) -> None:
except ShellDBError as e:
if e.code == 2003:
logger.exception(server_exceptions.ConnectionError_.MESSAGE)
raise server_exceptions.ConnectionError_
raise server_exceptions.ConnectionError from None
else:
logger.exception(
f"Failed to run MySQL Shell script:\n{script}\n\nMySQL client error {e.code}\nMySQL Shell traceback:\n{e.traceback_message}\n"
Expand All @@ -114,7 +114,7 @@ def _run_sql(self, sql_statements: typing.List[str]) -> None:
_jinja_env.get_template("run_sql.py.jinja").render(statements=sql_statements)
)

def _get_attributes(self, additional_attributes: dict = None) -> str:
def _get_attributes(self, additional_attributes: typing.Optional[dict] = None) -> str:
"""Attributes for (MySQL) users created by this charm

If the relation with the MySQL charm is broken, the MySQL charm will use this attribute
Expand Down Expand Up @@ -163,7 +163,7 @@ def get_mysql_router_user_for_unit(
again.
"""
logger.debug(f"Getting MySQL Router user for {unit_name=}")
output_file = self._container.path("/tmp/mysqlsh_output.json")
output_file = self._container.path("/tmp/mysqlsh_output.json") # noqa: S108
self._run_code(
_jinja_env.get_template("get_mysql_router_user_for_unit.py.jinja").render(
username=self.username,
Expand Down Expand Up @@ -210,7 +210,7 @@ def delete_user(self, username: str, *, must_exist=True) -> None:
def is_router_in_cluster_set(self, router_id: str) -> bool:
"""Check if MySQL Router is part of InnoDB ClusterSet."""
logger.debug(f"Checking if {router_id=} in cluster set")
output_file = self._container.path("/tmp/mysqlsh_output.json")
output_file = self._container.path("/tmp/mysqlsh_output.json") # noqa: S108
self._run_code(
_jinja_env.get_template("get_routers_in_cluster_set.py.jinja").render(
output_filepath=output_file.relative_to_container
Expand All @@ -226,7 +226,7 @@ def is_router_in_cluster_set(self, router_id: str) -> bool:


_jinja_env = jinja2.Environment(
autoescape=False,
autoescape=False, # noqa: S701
trim_blocks=True,
loader=jinja2.FileSystemLoader(pathlib.Path(__file__).parent / "templates"),
undefined=jinja2.StrictUndefined,
Expand Down
2 changes: 1 addition & 1 deletion src/relations/cos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class COSRelation:
_PEER_RELATION_NAME = "cos"

MONITORING_USERNAME = "monitoring"
_MONITORING_PASSWORD_KEY = "monitoring-password"
_MONITORING_PASSWORD_KEY = "monitoring-password" # noqa: S105

_TRACING_PROTOCOL = "otlp_http"

Expand Down
25 changes: 8 additions & 17 deletions src/relations/database_provides.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

"""Relation(s) to one or more application charms"""

import contextlib
import logging
import typing

Expand Down Expand Up @@ -221,30 +222,24 @@ def __init__(self, charm_: "abstract_charm.MySQLRouterCharm") -> None:
def _shared_users(self) -> typing.List[_RelationWithSharedUser]:
shared_users = []
for relation in self._interface.relations:
try:
with contextlib.suppress(_UserNotShared):
shared_users.append(
_RelationWithSharedUser(relation=relation, interface=self._interface)
)
except _UserNotShared:
pass
return shared_users

def external_connectivity(self, event) -> bool:
"""Whether any of the relations are marked as external."""
requested_users = []
for relation in self._interface.relations:
try:
with contextlib.suppress(
_RelationBreaking, remote_databag.IncompleteDatabag, _UnsupportedExtraUserRole
):
requested_users.append(
_RelationThatRequestedUser(
relation=relation, interface=self._interface, event=event
)
)
except (
_RelationBreaking,
remote_databag.IncompleteDatabag,
_UnsupportedExtraUserRole,
):
pass
return any(relation.external_connectivity for relation in requested_users)

def update_endpoints(
Expand Down Expand Up @@ -285,18 +280,14 @@ def reconcile_users(
)
requested_users = []
for relation in self._interface.relations:
try:
with contextlib.suppress(
_RelationBreaking, remote_databag.IncompleteDatabag, _UnsupportedExtraUserRole
):
requested_users.append(
_RelationThatRequestedUser(
relation=relation, interface=self._interface, event=event
)
)
except (
_RelationBreaking,
remote_databag.IncompleteDatabag,
_UnsupportedExtraUserRole,
):
pass
logger.debug(f"State of reconcile users {requested_users=}, {self._shared_users=}")
for relation in requested_users:
if relation not in self._shared_users:
Expand Down
2 changes: 1 addition & 1 deletion src/relations/database_requires.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def __init__(self, *, host: str, port: str, username: str):
self.host = host
self.port = port
self.username = username
self.password = "***"
self.password = "***" # noqa: S105


class CompleteConnectionInformation(ConnectionInformation):
Expand Down
18 changes: 5 additions & 13 deletions src/relations/deprecated_shared_db_database_provides.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,18 @@
Uses DEPRECATED "mysql-shared" relation interface
"""

import contextlib
import logging
import typing

import ops

import mysql_shell
import relations.remote_databag as remote_databag
import status_exception

if typing.TYPE_CHECKING:
import abstract_charm
import status_exception


class LogPrefix(logging.LoggerAdapter):
Expand Down Expand Up @@ -231,16 +232,14 @@ def _update_unit_databag(self, _) -> None:
logger.debug("Synchronizing unit databags")
requested_users = []
for relation in self._relations:
try:
with contextlib.suppress(remote_databag.IncompleteDatabag):
requested_users.append(
_UnitThatNeedsUser(
relation=relation,
unit=self._charm.unit,
peer_relation_app_databag=self._peer_app_databag,
)
)
except remote_databag.IncompleteDatabag:
pass
for relation in requested_users:
if password := self._peer_app_databag.get(relation.peer_databag_password_key):
relation.set_databag(password=password)
Expand All @@ -253,15 +252,13 @@ def _update_unit_databag(self, _) -> None:
def _shared_users(self) -> typing.List[_RelationWithSharedUser]:
shared_users = []
for relation in self._relations:
try:
with contextlib.suppress(_UserNotShared):
shared_users.append(
_RelationWithSharedUser(
relation=relation,
peer_relation_app_databag=self._peer_app_databag,
)
)
except _UserNotShared:
pass
return shared_users

def reconcile_users(
Expand All @@ -279,7 +276,7 @@ def reconcile_users(
logger.debug(f"Reconciling users {event=}")
requested_users = []
for relation in self._relations:
try:
with contextlib.suppress(_RelationBreaking, remote_databag.IncompleteDatabag):
requested_users.append(
_RelationThatRequestedUser(
relation=relation,
Expand All @@ -288,11 +285,6 @@ def reconcile_users(
event=event,
)
)
except (
_RelationBreaking,
remote_databag.IncompleteDatabag,
):
pass
logger.debug(f"State of reconcile users {requested_users=}, {self._shared_users=}")
for relation in requested_users:
if relation not in self._shared_users:
Expand Down
4 changes: 3 additions & 1 deletion src/relations/remote_databag.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,6 @@ def __getitem__(self, key):
logger.debug(
f"Required {key=} missing from databag for {self._app_name=} on {self._endpoint_name=}"
)
raise IncompleteDatabag(app_name=self._app_name, endpoint_name=self._endpoint_name)
raise IncompleteDatabag(
app_name=self._app_name, endpoint_name=self._endpoint_name
) from None
Loading
Loading