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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

flake8_to_ruff: support isort options #2082

Merged
merged 11 commits into from
Jan 22, 2023
20 changes: 14 additions & 6 deletions flake8_to_ruff/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,22 @@ fn main() -> Result<()> {
let config = ini.load(cli.file).map_err(|msg| anyhow::anyhow!(msg))?;

// Read the pyproject.toml file.
let black = cli
.pyproject
.map(flake8_to_ruff::parse_black_options)
.transpose()?
.flatten();
let (black, isort) = match cli.pyproject {
Some(path) => {
let black = flake8_to_ruff::parse_black_options(&path)?;
let isort = flake8_to_ruff::parse_isort_options(&path)?;
(black, isort)
}
None => (None, None),
};

let external_config = flake8_to_ruff::ExternalConfig {
black: black.as_ref(),
isort: isort.as_ref(),
};

// Create Ruff's pyproject.toml section.
let pyproject = flake8_to_ruff::convert(&config, black.as_ref(), cli.plugin)?;
let pyproject = flake8_to_ruff::convert(&config, &external_config, cli.plugin)?;
println!("{}", toml_edit::easy::to_string_pretty(&pyproject)?);

Ok(())
Expand Down
12 changes: 2 additions & 10 deletions src/flake8_to_ruff/black.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use serde::{Deserialize, Serialize};

use crate::settings::types::PythonVersion;

use super::pyproject::Pyproject;

#[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Default)]
pub struct Black {
#[serde(alias = "line-length", alias = "line_length")]
Expand All @@ -15,16 +17,6 @@ pub struct Black {
pub target_version: Option<Vec<PythonVersion>>,
}

#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
struct Tools {
black: Option<Black>,
}

#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
struct Pyproject {
tool: Option<Tools>,
}

pub fn parse_black_options<P: AsRef<Path>>(path: P) -> Result<Option<Black>> {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have one parse function that returns ExternalConfig. Right now, we're parsing the TOML twice into Pyproject, then extracting tool.black and tool.isort respectively, then merging them back into a single struct.

Copy link
Member

Choose a reason for hiding this comment

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

(Or, one parse function that returns Pyproject to main.rs, then create ExternalConfig from Pyproject.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yep that makes sense.

Tools has to own black/isort presumably for serde (de)serialization. I'm trying to do something like:

pub fn parse<'a, P: AsRef<Path>>(path: P) -> Result<ExternalConfig<'a>> {
    let contents = std::fs::read_to_string(path)?;
    let pyproject = toml_edit::easy::from_str::<Pyproject>(&contents)?;
    Ok(pyproject
        .tool
        .map(|tool| ExternalConfig {
            black: tool.black.as_ref(),
            isort: tool.isort.as_ref(),
        })
        .unwrap_or_default())
}

but the tool.{black,isort}.as_ref() lines complain because we're referencing tool.{black,isort} which is not owned by the current function. Any ideas? :)

Or, alternatively, if flake8_to_ruff::parse returns Pyproject and we try and produce an ExternalConfig from main.rs:

let pyproject = cli.pyproject.map(flake8_to_ruff::parse).transpose()?;
let external_config = pyproject
    .map(|pyproject| {
        pyproject
            .tool
            .map(|tool| ExternalConfig {
                // these lines aren't happy
                black: tool.black.as_ref(),
                isort: tool.isort.as_ref(),
            })
            .unwrap_or_default()
    })
    .unwrap_or_default();

Copy link
Contributor

Choose a reason for hiding this comment

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

The signature doesn't make sense:

pub fn parse<'a, P: AsRef<Path>>(path: P) -> Result<ExternalConfig<'a>> {

The liftetime in the return type has to come from somewhere ... you can either change the input to text: &'a str or make the return type owned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @not-my-profile, I've changed flake8_to_ruff::parse to return Pyproject instead, which circumvents the above issue.

The current issue is mapping out of Tools (where black/isort are owned) into ExternalConfig (where black/isort are references).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to work, but is there a preferred (more idiomatic) way?

let mut external_config = ExternalConfig::default();
let pyproject = cli.pyproject.map(flake8_to_ruff::parse).transpose()?;
if let Some(pyproject) = &pyproject {
    if let Some(tool) = &pyproject.tool {
        external_config = ExternalConfig {
            black: tool.black.as_ref(),
            isort: tool.isort.as_ref(),
        };
    }
}

Edit: maybe this?

let external_config = pyproject
    .as_ref()
    .map(|pyproject| {
        pyproject
            .tool
            .as_ref()
            .map(|tool| ExternalConfig {
                black: tool.black.as_ref(),
                isort: tool.isort.as_ref(),
            })
            .unwrap_or_default()
    })
    .unwrap_or_default();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented suggestions here @charliermarsh: 4127eb4 馃憤馃徏

Copy link
Member

Choose a reason for hiding this comment

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

Awesome -- will review in a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! I tweaked it a little bit to remove some levels of nesting by using .and_then.

let contents = std::fs::read_to_string(path)?;
Ok(toml_edit::easy::from_str::<Pyproject>(&contents)?
Expand Down
34 changes: 24 additions & 10 deletions src/flake8_to_ruff/converter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::collections::{BTreeSet, HashMap};
use anyhow::Result;
use colored::Colorize;

use super::black::Black;
use super::external_config::ExternalConfig;
use super::plugin::Plugin;
use super::{parser, plugin};
use crate::registry::RuleSelector;
Expand All @@ -23,7 +23,7 @@ use crate::warn_user;

pub fn convert(
config: &HashMap<String, HashMap<String, Option<String>>>,
black: Option<&Black>,
external_config: &ExternalConfig,
plugins: Option<Vec<Plugin>>,
) -> Result<Pyproject> {
// Extract the Flake8 section.
Expand Down Expand Up @@ -377,7 +377,7 @@ pub fn convert(
}

// Extract any settings from the existing `pyproject.toml`.
if let Some(black) = black {
if let Some(black) = &external_config.black {
if let Some(line_length) = &black.line_length {
options.line_length = Some(*line_length);
}
Expand All @@ -389,6 +389,19 @@ pub fn convert(
}
}

if let Some(isort) = &external_config.isort {
if let Some(src_paths) = &isort.src_paths {
match options.src.as_mut() {
Some(src) => {
src.extend(src_paths.clone());
}
None => {
options.src = Some(src_paths.clone());
}
}
}
}

// Create the pyproject.toml.
Ok(Pyproject::new(options))
}
Expand All @@ -401,6 +414,7 @@ mod tests {

use super::super::plugin::Plugin;
use super::convert;
use crate::flake8_to_ruff::ExternalConfig;
use crate::registry::RuleSelector;
use crate::rules::pydocstyle::settings::Convention;
use crate::rules::{flake8_quotes, pydocstyle};
Expand All @@ -411,7 +425,7 @@ mod tests {
fn it_converts_empty() -> Result<()> {
let actual = convert(
&HashMap::from([("flake8".to_string(), HashMap::default())]),
None,
&ExternalConfig::default(),
None,
)?;
let expected = Pyproject::new(Options {
Expand Down Expand Up @@ -475,7 +489,7 @@ mod tests {
"flake8".to_string(),
HashMap::from([("max-line-length".to_string(), Some("100".to_string()))]),
)]),
None,
&ExternalConfig::default(),
Some(vec![]),
)?;
let expected = Pyproject::new(Options {
Expand Down Expand Up @@ -539,7 +553,7 @@ mod tests {
"flake8".to_string(),
HashMap::from([("max_line_length".to_string(), Some("100".to_string()))]),
)]),
None,
&ExternalConfig::default(),
Some(vec![]),
)?;
let expected = Pyproject::new(Options {
Expand Down Expand Up @@ -603,7 +617,7 @@ mod tests {
"flake8".to_string(),
HashMap::from([("max_line_length".to_string(), Some("abc".to_string()))]),
)]),
None,
&ExternalConfig::default(),
Some(vec![]),
)?;
let expected = Pyproject::new(Options {
Expand Down Expand Up @@ -667,7 +681,7 @@ mod tests {
"flake8".to_string(),
HashMap::from([("inline-quotes".to_string(), Some("single".to_string()))]),
)]),
None,
&ExternalConfig::default(),
Some(vec![]),
)?;
let expected = Pyproject::new(Options {
Expand Down Expand Up @@ -739,7 +753,7 @@ mod tests {
Some("numpy".to_string()),
)]),
)]),
None,
&ExternalConfig::default(),
Some(vec![Plugin::Flake8Docstrings]),
)?;
let expected = Pyproject::new(Options {
Expand Down Expand Up @@ -810,7 +824,7 @@ mod tests {
"flake8".to_string(),
HashMap::from([("inline-quotes".to_string(), Some("single".to_string()))]),
)]),
None,
&ExternalConfig::default(),
None,
)?;
let expected = Pyproject::new(Options {
Expand Down
7 changes: 7 additions & 0 deletions src/flake8_to_ruff/external_config.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
use super::{black::Black, isort::Isort};

#[derive(Default)]
pub struct ExternalConfig<'a> {
pub black: Option<&'a Black>,
pub isort: Option<&'a Isort>,
}
22 changes: 22 additions & 0 deletions src/flake8_to_ruff/isort.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
//! Extract isort configuration settings from a pyproject.toml.

use std::path::Path;

use anyhow::Result;
use serde::{Deserialize, Serialize};

use super::pyproject::Pyproject;

/// The [isort configuration](https://pycqa.github.io/isort/docs/configuration/config_files.html).
#[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Default)]
pub struct Isort {
#[serde(alias = "src-paths", alias = "src_paths")]
pub src_paths: Option<Vec<String>>,
}

pub fn parse_isort_options<P: AsRef<Path>>(path: P) -> Result<Option<Isort>> {
let contents = std::fs::read_to_string(path)?;
Ok(toml_edit::easy::from_str::<Pyproject>(&contents)?
.tool
.and_then(|tool| tool.isort))
}
5 changes: 5 additions & 0 deletions src/flake8_to_ruff/mod.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
mod black;
mod converter;
mod external_config;
mod isort;
mod parser;
mod plugin;
mod pyproject;

pub use black::parse_black_options;
pub use converter::convert;
pub use external_config::ExternalConfig;
pub use isort::parse_isort_options;
pub use plugin::Plugin;
14 changes: 14 additions & 0 deletions src/flake8_to_ruff/pyproject.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
use serde::{Deserialize, Serialize};

use super::{black::Black, isort::Isort};

#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
pub struct Tools {
pub black: Option<Black>,
pub isort: Option<Isort>,
}

#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
pub struct Pyproject {
pub tool: Option<Tools>,
}