PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+1, -3, 0 error(s))

- disnake/utils.py:1137:21: S307 Use of possibly insecure function; consider using `ast.literal_eval`

- dev/stats/get_important_pr_candidates.py:131:32: PGH001 No builtin `eval()` allowed

- Packs/Malwr/Integrations/Malwr/Malwr.py:96:18: PGH001 No builtin `eval()` allowed

+ ibis/common/typing.py:197:69: RUF100 Unused `noqa` directive (non-enabled: `S307`)

Is this a breaking change? What do we need to document in the changelog?

Yes and no...

If users have PGH001 explicitly selected or ignored in their configuration file, we'll show a warning but automatically redirect it to S307.

If users have # noqa: PGH001, we'll similarly warn-and-redirect to S307.

The main problem here is that if users have PGH in their select, but not S, this rule will no longer run on their code. That's kind of an issue, because the rule is basically being silently turned off (and is the source of the ecosystem changes).

I'm not sure how best to resolve -- any ideas?

@zanieb - This one actually might be interesting to you too (especially my comment about regarding redirects).

We really should implement proper rule aliasing, this getting more and more unwieldy as we add new rules. It's really surprising when a new user migrates a flake8-plugin to ruff only to find they also have to enable a bunch of rules in different other categories to make functionality of the original plugin.

The main problem here is that if users have PGH in their select, but not S, this rule will no longer run on their code. That's kind of an issue, because the rule is basically being silently turned off

This sounds like a breaking change to me. Proper aliasing does sound like the better solution and probably something we need for recategorization anyway?

@zanieb Agree, public rule aliasing is probably one of the largest painpoints right now and it will only become a bigger maintenance burden on us later the more rules get added without this feature.

Separate from the larger conversation around rule aliasing (see my comment in the related issue), we should consider removing this whole category and marking it as a breaking change. There are so few rules, multiple of them are duplicates of other existing rules, and there's no semantic relationship between the various rules.

Proper aliasing does sound like the better solution and probably something we need for recategorization anyway?

I think there are ways for us to do a recategorization without aliasing, because it doesn't require that we support multiple codes at once for a given rule, only that we provide tooling to remap from existing to updated codes and categories.

Should we remove the whole category instead of this pull request? Either way I think we should probably gate removal or this alias with preview.

I’m in favor of that.

Closing in favor of #8931