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

feat(fmt): add support for configuration file #11944

Merged
merged 15 commits into from
Sep 13, 2021

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Sep 7, 2021

Example config with all supported options:

{
    "fmt": {
        "files": {
            "include": ["src/"],
            "exclude": ["src/testdata/"]
        },
        "options": {
            "useTabs": true,
            "lineWidth": 80,
            "indentWidth": 4,
            "singleQuote": true,
            "textWrap": "preserve"
        }
    }
}

Closes #11857

@bartlomieju bartlomieju added this to the 1.14.0 milestone Sep 7, 2021
pub line_width: Option<u32>,
pub indent_width: Option<u8>,
pub single_quote: Option<bool>,
pub text_wrap: Option<TextWrap>,
Copy link
Member

Choose a reason for hiding this comment

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

The reason this is called text_wrap in dprint-plugin-markdown is because it controls how the Text node wraps. Since this is a top level config and not markdown specific, I think we should call this prose_wrap to align with prettier.

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'm fine with changing name, but I'm unsure about that it's not Markdown specific - I haven't found a setting in dprint-plugin-typescript that would be relevant for this setting. Can you elaborate?

@bartlomieju
Copy link
Member Author

This PR is still missing tests and I will add them tomorrow. @kitsonk can you please review the changes to LSP?

@bartlomieju bartlomieju changed the title [WIP] feat(fmt): add support for configuration file feat(fmt): add support for configuration file Sep 13, 2021
Comment on lines 3211 to 3216
json!({
"textDocument": {
"uri": "file:https:///a/file.ts"
},
"options": {}
}),
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 have trouble with this test, I'm getting following error:

running 1 test
test integration::lsp::lsp_format_with_config ... FAILED

failures:

---- integration::lsp::lsp_format_with_config stdout ----
Some(
    LspResponseError {
        code: -32602,
        message: "missing field `tabSize`",
        data: None,
    },
)
thread 'integration::lsp::lsp_format_with_config' panicked at 'assertion failed: maybe_err.is_none()', cli/tests/integration/lsp_tests.rs:3221:3
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Should I just pass fake data here and count on the fact that config file should take precedence over these options?

CC @kitsonk

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 that's fine to provide some fake data. The editor will have a "tab size" for a file that might not be synced with the formatting settings.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First pass at "deno fmt" config
2 participants