Enforce ruff/flake8-bugbear rules (B)

[DimitriPapadopoulos]

Apply Repo-Review suggestion RF101:
https://learn.scientific-python.org/development/guides/repo-review/?repo=skrub-data%2Fskrub&ref=HEAD

[rcap107]

Hi, thanks for the PR.

Overall, the missing asserts are useful (really wondering why they're missing in the first place). Other changes do not look like they improve the code in a meaningful way (B007, B004, B010); we can stil implement the changes, however.

What I am really concerned with is actually enforcing the bugbear rules. Looking at the docs, a lot of them look fairly esoteric and definitely not something most people (especially new contributors) would be familiar with. Enforcing those rules would make contributing far more complicated than it already is.

As a result, those rules should not be added to the default linting. I'll add them to my own ruleset, but it should not be in the default pyproject.

[rcap107]

selctors.all() isn't selecting all columns, it's selecting the all() selector, which selects all columns.

Suggested change

	_SELECTORS = selectors.all()
	# By default, select all columns
	_SELECT_ALL_COLUMNS = selectors.all()

[rcap107]

Same change in all the other files

[rcap107]

Same change in all the other files

[rcap107]

Suggested change

	_SELECTORS = selectors.all()
	_SELECT_ALL_COLUMNS = selectors.all()

[rcap107]

As a more general comment for this and all the other PRs that you opened, formatting rules should be chosen by the maintainers of the project, rather than added in a dozen of separate PRs that need to be reviewed one by one.

In this PR, a large part of the diff is "whatever", the _SELECTORS change was made while misunderstanding the code, and while reviewing it I ended up misunderstanding the code myself, meaning that if it had been a more critical change it would have introduced a bug in the code.

The suggested rules are way too restrictive for new contributors, so that part of the PR is also not going to be merged, and should definitely have been discussed in an issue rather than added here.

And this is only one of 20 very similar PRs that have been opened, and that I have to spend time on reviewing, making sure that none of the small changes that have been made across a bunch of files can lead to issues of some kind.

In the future, please consider the time and effort that goes into reviewing all of this. I will still try to review the PRs that have already been opened, and track the suggested changes in an issue for discussion and possible implementation.

The proper way of contributing to the repo would be opening a meta-issue that collects a series of sub-issues, so that it is possible to decide whether specific changes are warranted. This would take less time than reviewing PRs, and would not risk introducing bugs in the code.

If it does not look like future PRs take this comment into consideration, I don't exclude closing any new PR like this without even looking at what's inside.

[DimitriPapadopoulos]

• I would recommend keeping B004, it does result in more correct code, although this should seldom be an issue in practice:

  Using hasattr is an unreliable mechanism for testing if an object is callable. If obj implements a custom __getattr__, or if its __call__ is itself not callable, you may get misleading results.

• B007 is indeed debatable.
• B010 improves code consistency and readability, but I can understand you'd rather ignore it to avoid frustrating new contributors.

[DimitriPapadopoulos]

Other changes do not look like they improve the code in a meaningful way (B007, B004, B010); we can stil implement the changes, however.

I have kept the changes in this PR, but these rules will be ignored in the future.

[DimitriPapadopoulos]

Still need to fix this new error in a test:

FAILED skrub/tests/test_multi_agg_joiner.py::test_default_cols[pandas-nullable-dtypes] - AssertionError: assert [['rating', '...', 'movieId']] == [['rating', '...ng', 'genre']]
  
  At index 1 diff: ['rating', 'genre', 'movieId'] != ['movieId', 'rating', 'genre']

Columns should be sorted in the same order as in the initial main_table, shouldn't they?

skrub/skrub/tests/test_multi_agg_joiner.py

Lines 9 to 19 in 20598ed

	@pytest.fixture
	def main_table():
	df = pd.DataFrame(
	{
	"userId": [1, 1, 1, 2, 2, 2],
	"movieId": [1, 3, 6, 318, 6, 1704],
	"rating": [4.0, 4.0, 4.0, 3.0, 2.0, 4.0],
	"genre": ["drama", "drama", "comedy", "sf", "comedy", "sf"],
	}
	)
	return df

[DimitriPapadopoulos]

Note that Ruff can automatically apply some rules, and pre-commit will enforce that without any additional work from contributors. Therefore, rules with automatic fixes are not a barrier, they result in consistent and readable code which can only help maintenance and new contributions.

Ruff provides automatic fixes for many B rules, including B004, B007, B010.