Skip to content

Comments

Promote lint. settings over top-level settings#9476

Merged
MichaReiser merged 3 commits intorelease/0.2.0from
promote-lint-options-over-top-level-settings
Jan 29, 2024
Merged

Promote lint. settings over top-level settings#9476
MichaReiser merged 3 commits intorelease/0.2.0from
promote-lint-options-over-top-level-settings

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jan 11, 2024

Summary

This PR marks the top-level lint settings as deprecated (schema only) and updates the website to document the options under the lint section (removes the top-level settings).

I had to make changes to the documentation generation to now support 3-level deep options

  • The documentation now shows the entire name for sub-sub-sections (e.g. lint.pydocstyle instead of pydocstyle) because sub-sub-sections are rendered at the same level as sub-sections.
  • The anchor link now contains the entire parent chain: lint.pycodestyle.xxx instead of pycodestyle.xxx.
  • The anchor link now use _ instead of - as a separator to disambiguate between the subsection lint.pydocstyle and option lint-pydocstyle

I feel undecided about one open question: Should we use the full setting name in READMEs or only the name? For example. Should we write lint.ignore or ignore? Or a more extreme example. Should we write max-complexityorlint.mccabe.max-complexity`. We currently use a mix of both:

  • The tutorial mainly uses the short names
  • Rules use the full name (until today without the lint prefix)

I changed the tutorial to use long names in most positions but it didn't always feel right.

Considerations

One downside of removing the top-level settings from the website is that Ruff's documentation isn't versioned. Users using an older version of ruff that only supports the global settings or use the global options might be confused why the options are missing.

A possible solution is adding aliasing support to our documentation generation, although that might be tricky.

IMO, removing is fine because the lint section has been supported since v0.0.292, which feels like a very long time ago.

Next steps

Emit deprecation messages when a configuration uses any deprecated top-level settings.

Test Plan

  • I built the documentation locally and verified that the lint options are only listed once under the lint section (which helps discoverability).
  • I read through the documentation and clicked through the setting options to verify that the anchor links work
  • I reviewed the schema changes and it marks the top level lint settings as deprecated.

image

Danger: Only merge before the 0.2.0 release

@MichaReiser MichaReiser added breaking Breaking API change configuration Related to settings and configuration and removed breaking Breaking API change labels Jan 11, 2024
@MichaReiser MichaReiser force-pushed the promote-lint-options-over-top-level-settings branch from 372b4d1 to 37df312 Compare January 19, 2024 13:56
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 19, 2024

CodSpeed Performance Report

Merging #9476 will not alter performance

⚠️ No base runs were found

Falling back to comparing promote-lint-options-over-top-level-settings (6a200c0) with main (50122d2)

Summary

✅ 30 untouched benchmarks

Comment on lines +329 to +333
- [`force-single-line`](settings.md#lint_isort_force-single-line)
- [`force-wrap-aliases`](settings.md#lint_isort_force-wrap-aliases)
- [`lines-after-imports`](settings.md#lint_isort_lines-after-imports)
- [`lines-between-types`](settings.md#lint_isort_lines-between-types)
- [`split-on-trailing-comma`](settings.md#lint_isort_split-on-trailing-comma)
Copy link
Member Author

Choose a reason for hiding this comment

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

An example where we don't use full qualified setting names.

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 ok to me.

@MichaReiser MichaReiser force-pushed the promote-lint-options-over-top-level-settings branch from 37df312 to d9d44d8 Compare January 19, 2024 14:12
@MichaReiser MichaReiser added this to the v0.2.0 milestone Jan 19, 2024
@MichaReiser MichaReiser marked this pull request as ready for review January 19, 2024 14:15
@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser force-pushed the promote-lint-options-over-top-level-settings branch from d9d44d8 to c9dbc2d Compare January 19, 2024 14:31
The set of enabled rules is controlled via the [`select`](settings.md#select),
[`extend-select`](settings.md#extend-select), and [`ignore`](settings.md#ignore) settings.
The set of enabled rules is controlled via the [`lint.select`](settings.md#lint_select),
[`lint.extend-select`](settings.md#lint_extend-select), and [`lint.ignore`](settings.md#lint_ignore) settings.
Copy link
Member

Choose a reason for hiding this comment

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

All the changes in here look reasonable to me.

///
/// ## Options
/// - `flake8-bugbear.extend-immutable-calls`
/// - `lint.flake8-bugbear.extend-immutable-calls`
Copy link
Member

Choose a reason for hiding this comment

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

Did you verify manually that these still work, when rendered in the rule documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I verified some of them ;)

.into_iter()
.chain(parents.iter().filter_map(|parent| parent.name()))
.chain(scope)
.join(".");
Copy link
Member

Choose a reason for hiding this comment

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

This all got so much simpler, where did the simplification come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

By writing simpler code 😆

I realised that the only difference between the two is whether we add the tool.ruff prefix or not. The fact that parents is now an iter made it more obvious to me that we could use iter.join over manually constructing the string.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

The changes you made (to the docs, etc.) generally look good to me. It's a shame that existing anchor links won't work any more, but it seems really hard to avoid. Thanks for cleaning this up.

@MichaReiser
Copy link
Member Author

The changes you made (to the docs, etc.) generally look good to me. It's a shame that existing anchor links won't work any more, but it seems really hard to avoid. Thanks for cleaning this up.

We could add backwards-compatible anchors. We could add backwards compatible ids if we want to keep anchor links from external websites working (I hope I managed to update all internal references).

@MichaReiser MichaReiser force-pushed the promote-lint-options-over-top-level-settings branch from c9dbc2d to 827f3a5 Compare January 22, 2024 08:51
@MichaReiser MichaReiser changed the base branch from main to release/0.2.0 January 29, 2024 17:16
@MichaReiser MichaReiser force-pushed the promote-lint-options-over-top-level-settings branch from 827f3a5 to b506995 Compare January 29, 2024 17:20
@MichaReiser MichaReiser merged commit e94f0f8 into release/0.2.0 Jan 29, 2024
@MichaReiser MichaReiser deleted the promote-lint-options-over-top-level-settings branch January 29, 2024 17:42
@zanieb zanieb mentioned this pull request Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

configuration Related to settings and configuration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants