Skip to content
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.

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

Refactor Options representation #7591

Merged
merged 1 commit into from
Sep 22, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion crates/ruff_cli/src/commands/config.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use anyhow::{anyhow, Result};

use ruff_workspace::options::Options;
use ruff_workspace::options_base::OptionsMetadata;

#[allow(clippy::print_stdout)]
pub(crate) fn config(key: Option<&str>) -> Result<()> {
match key {
None => print!("{}", Options::metadata()),
Some(key) => match Options::metadata().get(key) {
Some(key) => match Options::metadata().find(key) {
None => {
return Err(anyhow!("Unknown option: {key}"));
}
Expand Down
6 changes: 2 additions & 4 deletions crates/ruff_dev/src/generate_docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use strum::IntoEnumIterator;
use ruff_diagnostics::AutofixKind;
use ruff_linter::registry::{Linter, Rule, RuleNamespace};
use ruff_workspace::options::Options;
use ruff_workspace::options_base::OptionsMetadata;

use crate::ROOT_DIR;

Expand Down Expand Up @@ -96,10 +97,7 @@ fn process_documentation(documentation: &str, out: &mut String) {
if let Some(rest) = line.strip_prefix("- `") {
let option = rest.trim_end().trim_end_matches('`');

assert!(
Options::metadata().get(option).is_some(),
"unknown option {option}"
);
assert!(Options::metadata().has(option), "unknown option {option}");

let anchor = option.replace('.', "-");
out.push_str(&format!("- [`{option}`][{option}]\n"));
Expand Down
105 changes: 72 additions & 33 deletions crates/ruff_dev/src/generate_options.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,68 @@
//! Generate a Markdown-compatible listing of configuration options for `pyproject.toml`.
//!
//! Used for <https://docs.astral.sh/ruff/settings/>.
use itertools::Itertools;
use std::fmt::Write;

use ruff_workspace::options::Options;
use ruff_workspace::options_base::{OptionEntry, OptionField};
use ruff_workspace::options_base::{OptionField, OptionSet, OptionsMetadata, Visit};

pub(crate) fn generate() -> String {
let mut output = String::new();
generate_set(&mut output, &Set::Toplevel(Options::metadata()));

output
}

fn generate_set(output: &mut String, set: &Set) {
writeln!(output, "### {title}\n", title = set.title()).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

writeln! with trailing \n looks strange

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree but everything else feels verbose just to get two newlines

Copy link
Member

Choose a reason for hiding this comment

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

(I typically do two writeln for this but don't feel strongly.)


let mut visitor = CollectOptionsVisitor::default();
set.metadata().record(&mut visitor);

let (mut fields, mut sets) = (visitor.fields, visitor.groups);

fields.sort_unstable_by(|(name, _), (name2, _)| name.cmp(name2));
sets.sort_unstable_by(|(name, _), (name2, _)| name.cmp(name2));
Comment on lines +24 to +25
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 don't know how I feel about sorting options. It disallows grouping options that are semantically related (all resolver settings)


// Generate the fields.
for (name, field) in &fields {
emit_field(output, name, field, set.name());
output.push_str("---\n\n");
}

// Generate all the sub-sets.
for (set_name, sub_set) in &sets {
generate_set(output, &Set::Named(set_name, *sub_set));
}
}

enum Set<'a> {
Toplevel(OptionSet),
Named(&'a str, OptionSet),
}

impl<'a> Set<'a> {
fn name(&self) -> Option<&'a str> {
match self {
Set::Toplevel(_) => None,
Set::Named(name, _) => Some(name),
}
}

fn title(&self) -> &'a str {
match self {
Set::Toplevel(_) => "Top-level",
Set::Named(name, _) => name,
}
}

fn metadata(&self) -> &OptionSet {
match self {
Set::Toplevel(set) => set,
Set::Named(_, set) => set,
}
}
}

fn emit_field(output: &mut String, name: &str, field: &OptionField, group_name: Option<&str>) {
// if there's a group name, we need to add it to the anchor
Expand Down Expand Up @@ -37,38 +96,18 @@ fn emit_field(output: &mut String, name: &str, field: &OptionField, group_name:
output.push('\n');
}

pub(crate) fn generate() -> String {
let mut output: String = "### Top-level\n\n".into();

let sorted_options: Vec<_> = Options::metadata()
.into_iter()
.sorted_by_key(|(name, _)| *name)
.collect();

// Generate all the top-level fields.
for (name, entry) in &sorted_options {
let OptionEntry::Field(field) = entry else {
continue;
};
emit_field(&mut output, name, field, None);
output.push_str("---\n\n");
}
#[derive(Default)]
struct CollectOptionsVisitor {
groups: Vec<(String, OptionSet)>,
fields: Vec<(String, OptionField)>,
}

// Generate all the sub-groups.
for (group_name, entry) in &sorted_options {
let OptionEntry::Group(fields) = entry else {
continue;
};
output.push_str(&format!("### {group_name}\n"));
output.push('\n');
for (name, entry) in fields.iter().sorted_by_key(|(name, _)| name) {
let OptionEntry::Field(field) = entry else {
continue;
};
emit_field(&mut output, name, field, Some(group_name));
output.push_str("---\n\n");
}
impl Visit for CollectOptionsVisitor {
fn record_set(&mut self, name: &str, group: OptionSet) {
self.groups.push((name.to_owned(), group));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The old implementation only supports one level of nesting. This would have become a problem with lint.pydocstyle.option where we have two level nesting.


output
fn record_field(&mut self, name: &str, field: OptionField) {
self.fields.push((name.to_owned(), field));
}
}
6 changes: 2 additions & 4 deletions crates/ruff_dev/src/generate_rules_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use ruff_diagnostics::AutofixKind;
use ruff_linter::registry::{Linter, Rule, RuleNamespace};
use ruff_linter::upstream_categories::UpstreamCategoryAndPrefix;
use ruff_workspace::options::Options;
use ruff_workspace::options_base::OptionsMetadata;

const FIX_SYMBOL: &str = "🛠️";
const PREVIEW_SYMBOL: &str = "🧪";
Expand Down Expand Up @@ -104,10 +105,7 @@ pub(crate) fn generate() -> String {
table_out.push('\n');
}

if Options::metadata()
.iter()
.any(|(name, _)| name == &linter.name())
{
if Options::metadata().has(linter.name()) {
table_out.push_str(&format!(
"For related settings, see [{}](settings.md#{}).",
linter.name(),
Expand Down
25 changes: 12 additions & 13 deletions crates/ruff_macros/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,11 @@ pub(crate) fn derive_impl(input: DeriveInput) -> syn::Result<proc_macro2::TokenS
};
}

let options_len = output.len();

Ok(quote! {

impl #ident {
pub const fn metadata() -> crate::options_base::OptionGroup {
const OPTIONS: [(&'static str, crate::options_base::OptionEntry); #options_len] = [#(#output),*];
crate::options_base::OptionGroup::new(&OPTIONS)
impl crate::options_base::OptionsMetadata for #ident {
fn record(visit: &mut dyn crate::options_base::Visit) {
#(#output);*
}
}
})
Expand Down Expand Up @@ -92,7 +89,7 @@ fn handle_option_group(field: &Field) -> syn::Result<proc_macro2::TokenStream> {
let kebab_name = LitStr::new(&ident.to_string().replace('_', "-"), ident.span());

Ok(quote_spanned!(
ident.span() => (#kebab_name, crate::options_base::OptionEntry::Group(#path::metadata()))
ident.span() => (visit.record_set(#kebab_name, crate::options_base::OptionSet::of::<#path>()))
))
}
_ => Err(syn::Error::new(
Expand Down Expand Up @@ -150,12 +147,14 @@ fn handle_option(
let kebab_name = LitStr::new(&ident.to_string().replace('_', "-"), ident.span());

Ok(quote_spanned!(
ident.span() => (#kebab_name, crate::options_base::OptionEntry::Field(crate::options_base::OptionField {
doc: &#doc,
default: &#default,
value_type: &#value_type,
example: &#example,
}))
ident.span() => {
visit.record_field(#kebab_name, crate::options_base::OptionField{
doc: &#doc,
default: &#default,
value_type: &#value_type,
example: &#example,
})
}
))
}

Expand Down
11 changes: 7 additions & 4 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use rustc_hash::{FxHashMap, FxHashSet};
use serde::{Deserialize, Serialize};
use strum::IntoEnumIterator;

use crate::options_base::{OptionsMetadata, Visit};
use ruff_linter::line_width::{LineLength, TabSize};
use ruff_linter::rules::flake8_pytest_style::settings::SettingsError;
use ruff_linter::rules::flake8_pytest_style::types;
Expand Down Expand Up @@ -2399,10 +2400,6 @@ pub enum FormatOrOutputFormat {
}

impl FormatOrOutputFormat {
pub const fn metadata() -> crate::options_base::OptionGroup {
FormatOptions::metadata()
}

pub const fn as_output_format(&self) -> Option<SerializationFormat> {
match self {
FormatOrOutputFormat::Format(_) => None,
Expand All @@ -2411,6 +2408,12 @@ impl FormatOrOutputFormat {
}
}

impl OptionsMetadata for FormatOrOutputFormat {
fn record(visit: &mut dyn Visit) {
FormatOptions::record(visit);
}
}

#[derive(
Debug, PartialEq, Eq, Default, Serialize, Deserialize, ConfigurationOptions, CombineOptions,
)]
Expand Down