flake8_to_ruff: support isort options

[shannonrothe]

Hey @charliermarsh 👋🏼 I've been learning Rust for a little bit but haven't contributed any PRs until this one, so excuse me if it's a little rough. I mostly followed what was already in place for picking up Black options in the flake8_to_ruff converter.

For the isort options, I've chosen to support only src_paths for now as a simple starting point (this could be extended upon).

Admittedly, I'm not super familiar with the Python ecosystem and the associated tools (isort/Black), so let me know if I'm missing anything obvious 👌🏼

See: #1749

[charliermarsh]

Thanks for getting involved!

[charliermarsh]

For posterity: relevant issue is #1749.

[shannonrothe]

For posterity: relevant issue is #1749.

Added to description 👌🏼

[not-my-profile]

I am not sure if this PR should close #1749 given that isort also has other settings not implemented by this PR.

[shannonrothe]

I am not sure if this PR should close #1749 given that isort also has other settings not implemented by this PR.

Fair enough, happy to remove from the description.

Another question - we could probably build off this PR to alleviate #1567 right? Is there a counterpart option in Ruff for the Line-Length isort option?

[charliermarsh]

Ah yeah, sorry, it shouldn't close #1749 -- it should just reference it, like "See: #1749" or similar.

[charliermarsh]

Another question - we could probably build off this PR to alleviate #1567 right? Is there a counterpart option in Ruff for the Line-Length isort option?

It's actually a bit different -- that issue references a setting that tells isort to sort the imports by their string length. isort does have a line-length setting, and we should extract that and pass it along to Ruff (we have a top-level option for that), but it wouldn't close that issue :)

[charliermarsh]

Relatedly, everything in this section of the configuration options can be extracted from the isort section in a pyproject.toml, like you're doing here for src.

[charliermarsh]

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.

[charliermarsh]

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

[shannonrothe]

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();

[not-my-profile]

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.

[shannonrothe]

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).

[shannonrothe]

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();

[shannonrothe]

Implemented suggestions here @charliermarsh: 4127eb4 👍🏼

[charliermarsh]

Awesome -- will review in a bit.

[charliermarsh]

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