-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Conversation
cli/config_file.rs
Outdated
pub line_width: Option<u32>, | ||
pub indent_width: Option<u8>, | ||
pub single_quote: Option<bool>, | ||
pub text_wrap: Option<TextWrap>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
This PR is still missing tests and I will add them tomorrow. @kitsonk can you please review the changes to LSP? |
cli/tests/integration/lsp_tests.rs
Outdated
json!({ | ||
"textDocument": { | ||
"uri": "file:///a/file.ts" | ||
}, | ||
"options": {} | ||
}), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Example config with all supported options:
Closes #11857