-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Implement EditorConfig support #1777
base: master
Are you sure you want to change the base?
Changes from all commits
aa65111
0d433fd
a9ee261
6950865
c120e35
86c0923
278d9f4
73a8f25
2bff8f2
0ec192a
ff3d631
d07440d
e177b85
c878e20
2d8fefa
7f57658
77dffd7
7de1bbe
5d53965
ff215a8
91af0f1
a4f9325
6fbc8af
a359bec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,6 +135,8 @@ pub struct Document { | |
|
||
/// Current indent style. | ||
pub indent_style: IndentStyle, | ||
/// Current tab width in columns. | ||
pub tab_width: usize, | ||
|
||
/// The document's default line ending. | ||
pub line_ending: LineEnding, | ||
|
@@ -457,6 +459,106 @@ where | |
*mut_ref = f(mem::take(mut_ref)); | ||
} | ||
|
||
/// Trait representing ways of configuring the document as it's being opened. | ||
trait ConfigureDocument { | ||
type Config; | ||
/// Loads document configuration for a file at `path`. | ||
fn load(&self, path: &Path) -> Result<Self::Config, Error>; | ||
/// Retrieves the encoding from a `Config`. | ||
fn encoding(config: &Self::Config) -> Option<&'static encoding::Encoding>; | ||
/// Retrieves the line ending from a `Config`. | ||
fn line_ending(config: &Self::Config) -> Option<LineEnding>; | ||
/// Applies any document configuration not handled by one of the other methods. | ||
fn configure_document(doc: &mut Document, config: Self::Config); | ||
} | ||
|
||
/// Document configuration strategy that uses fallback auto-detection as a first resort. | ||
#[derive(Clone, Copy, Debug, Default)] | ||
struct Autodetect; | ||
/// Document configuration strategy that loads configuration from `.editorconfig` files. | ||
#[derive(Clone, Copy, Debug, Default)] | ||
struct EditorConfig; | ||
|
||
impl ConfigureDocument for Autodetect { | ||
type Config = (); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how this should be an associate trait, these data could be put within the struct itself. |
||
|
||
fn load(&self, _: &Path) -> Result<Self::Config, Error> { | ||
Ok(()) | ||
} | ||
|
||
fn encoding(_: &Self::Config) -> Option<&'static encoding::Encoding> { | ||
None | ||
} | ||
|
||
fn line_ending(_: &Self::Config) -> Option<LineEnding> { | ||
None | ||
} | ||
|
||
fn configure_document(doc: &mut Document, _: Self::Config) { | ||
doc.detect_indent(); | ||
} | ||
} | ||
|
||
impl ConfigureDocument for EditorConfig { | ||
type Config = ec4rs::Properties; | ||
|
||
fn load(&self, path: &Path) -> Result<Self::Config, Error> { | ||
let mut config = ec4rs::properties_of(path)?; | ||
config.use_fallbacks(); | ||
Ok(config) | ||
} | ||
|
||
fn encoding(config: &Self::Config) -> Option<&'static encoding::Encoding> { | ||
use ec4rs::property::Charset; | ||
use encoding::Encoding; | ||
config | ||
.get_raw::<Charset>() | ||
.filter_unset() | ||
.into_result() | ||
.ok() | ||
.and_then(|string| Encoding::for_label(string.to_lowercase().as_bytes())) | ||
} | ||
|
||
fn line_ending(config: &Self::Config) -> Option<LineEnding> { | ||
use ec4rs::property::EndOfLine; | ||
match config.get::<EndOfLine>() { | ||
Ok(EndOfLine::Lf) => Some(LineEnding::LF), | ||
Ok(EndOfLine::CrLf) => Some(LineEnding::Crlf), | ||
#[cfg(feature = "unicode-lines")] | ||
Ok(EndOfLine::Cr) => Some(LineEnding::CR), | ||
_ => None, | ||
} | ||
} | ||
|
||
fn configure_document(doc: &mut Document, settings: Self::Config) { | ||
use ec4rs::property::{IndentSize, IndentStyle as IndentStyleEc, TabWidth}; | ||
match settings.get::<IndentStyleEc>() { | ||
Ok(IndentStyleEc::Tabs) => doc.indent_style = IndentStyle::Tabs, | ||
Ok(IndentStyleEc::Spaces) => { | ||
let spaces = if let Ok(IndentSize::Value(cols)) = settings.get::<IndentSize>() { | ||
cols | ||
} else { | ||
doc.tab_width | ||
}; | ||
// Constrain spaces to only supported values for IndentStyle::Spaces. | ||
let spaces_u8 = if spaces > 8 { | ||
8u8 | ||
} else if spaces > 0 { | ||
// Shouldn't panic. Overflow cases are covered by the above branch. | ||
spaces as u8 | ||
} else { | ||
4u8 | ||
}; | ||
doc.indent_style = IndentStyle::Spaces(spaces_u8); | ||
} | ||
_ => doc.detect_indent(), | ||
} | ||
if let Ok(TabWidth::Value(width)) = settings.get::<TabWidth>() { | ||
doc.tab_width = width; | ||
} | ||
} | ||
} | ||
|
||
use helix_lsp::lsp; | ||
use url::Url; | ||
|
||
|
@@ -479,6 +581,7 @@ impl Document { | |
inlay_hints: HashMap::default(), | ||
inlay_hints_oudated: false, | ||
indent_style: DEFAULT_INDENT, | ||
tab_width: 4, | ||
line_ending: DEFAULT_LINE_ENDING, | ||
restore_cursor: false, | ||
syntax: None, | ||
|
@@ -511,25 +614,80 @@ impl Document { | |
config_loader: Option<Arc<syntax::Loader>>, | ||
config: Arc<dyn DynAccess<Config>>, | ||
) -> Result<Self, Error> { | ||
// Open the file if it exists, otherwise assume it is a new file (and thus empty). | ||
let (rope, encoding) = if path.exists() { | ||
let mut file = | ||
std::fs::File::open(path).context(format!("unable to open {:?}", path))?; | ||
from_reader(&mut file, encoding)? | ||
if config.load().editorconfig { | ||
Document::open_with_cfg(&EditorConfig, path, encoding, config_loader, config) | ||
} else { | ||
let encoding = encoding.unwrap_or(encoding::UTF_8); | ||
(Rope::from(DEFAULT_LINE_ENDING.as_str()), encoding) | ||
Document::open_with_cfg(&Autodetect, path, encoding, config_loader, config) | ||
} | ||
Comment on lines
+617
to
+621
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So based on my previous suggestion, this block should do let doc_config = if editorconfig {
// editorconfig load file
} else {
// normal load file
};
...
// other blocks in `open`
// then configure document
match doc_config {
... Would this be simpler? |
||
} | ||
// TODO: async fn? | ||
fn open_with_cfg<C: ConfigureDocument>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't both of this be in the same |
||
doc_config_loader: &C, | ||
path: &Path, | ||
encoding: Option<&'static encoding::Encoding>, | ||
config_loader: Option<Arc<syntax::Loader>>, | ||
config: Arc<dyn DynAccess<Config>>, | ||
) -> Result<Self, Error> { | ||
let (rope, doc_config, encoding, line_ending) = match std::fs::File::open(path) { | ||
// Match errors that we should NOT ignore. | ||
Err(e) if !matches!(e.kind(), std::io::ErrorKind::NotFound) => { | ||
return Err(e).context(format!("unable to open {:?}", path)); | ||
} | ||
result => { | ||
// Load doc_config for the file at this path. | ||
let doc_config = doc_config_loader | ||
.load(path) | ||
.map_err(|e| log::warn!("unable to load document config for {:?}: {}", path, e)) | ||
.ok(); | ||
// Override the doc_config encoding. | ||
let encoding = encoding.or_else(|| doc_config.as_ref().and_then(C::encoding)); | ||
if let Ok(mut file) = result { | ||
let (rope, encoding) = from_reader(&mut file, encoding)?; | ||
(rope, doc_config, Some(encoding), None) | ||
} else { | ||
// If we're here, the error can be recovered from. | ||
// Treat this as a new file. | ||
let line_ending = doc_config | ||
.as_ref() | ||
.and_then(C::line_ending) | ||
.unwrap_or(DEFAULT_LINE_ENDING); | ||
( | ||
Rope::from(line_ending.as_str()), | ||
doc_config, | ||
encoding, | ||
Some(line_ending), | ||
) | ||
} | ||
} | ||
}; | ||
|
||
let mut doc = Self::from(rope, Some(encoding), config); | ||
let mut doc = Self::from(rope, encoding, config); | ||
|
||
// set the path and try detecting the language | ||
doc.set_path(Some(path))?; | ||
if let Some(loader) = config_loader { | ||
doc.detect_language(loader); | ||
} | ||
|
||
doc.detect_indent_and_line_ending(); | ||
// Set the tab witdh from language config, allowing it to be overridden later. | ||
// Default of 4 is set in Document::from. | ||
if let Some(indent) = doc | ||
.language_config() | ||
.and_then(|config| config.indent.as_ref()) | ||
{ | ||
doc.tab_width = indent.tab_width | ||
} | ||
|
||
if let Some(doc_config) = doc_config { | ||
C::configure_document(&mut doc, doc_config); | ||
} else { | ||
Autodetect::configure_document(&mut doc, ()); | ||
} | ||
|
||
if let Some(line_ending) = line_ending { | ||
doc.line_ending = line_ending; | ||
} else { | ||
doc.detect_line_ending() | ||
} | ||
|
||
Ok(doc) | ||
} | ||
|
@@ -754,17 +912,28 @@ impl Document { | |
} | ||
|
||
/// Detect the indentation used in the file, or otherwise defaults to the language indentation | ||
/// configured in `languages.toml`, with a fallback to tabs if it isn't specified. Line ending | ||
/// is likewise auto-detected, and will fallback to the default OS line ending. | ||
pub fn detect_indent_and_line_ending(&mut self) { | ||
/// configured in `languages.toml`, with a fallback to tabs if it isn't specified. | ||
pub fn detect_indent(&mut self) { | ||
self.indent_style = auto_detect_indent_style(&self.text).unwrap_or_else(|| { | ||
self.language_config() | ||
.and_then(|config| config.indent.as_ref()) | ||
.map_or(DEFAULT_INDENT, |config| IndentStyle::from_str(&config.unit)) | ||
}); | ||
} | ||
|
||
/// Detect the line endings used in the file, with a fallback to the default OS line ending. | ||
pub fn detect_line_ending(&mut self) { | ||
self.line_ending = auto_detect_line_ending(&self.text).unwrap_or(DEFAULT_LINE_ENDING); | ||
} | ||
|
||
/// Detect the indentation used in the file, or otherwise defaults to the language indentation | ||
/// configured in `languages.toml`, with a fallback to tabs if it isn't specified. Line ending | ||
/// is likewise auto-detected, and will fallback to the default OS line ending. | ||
pub fn detect_indent_and_line_ending(&mut self) { | ||
self.detect_indent(); | ||
self.detect_line_ending(); | ||
} | ||
|
||
/// Reload the document from its path. | ||
pub fn reload( | ||
&mut self, | ||
|
@@ -1301,9 +1470,7 @@ impl Document { | |
|
||
/// The width that the tab character is rendered at | ||
pub fn tab_width(&self) -> usize { | ||
self.language_config() | ||
.and_then(|config| config.indent.as_ref()) | ||
.map_or(4, |config| config.tab_width) // fallback to 4 columns | ||
self.tab_width | ||
} | ||
|
||
// The width (in spaces) of a level of indentation. | ||
|
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.
One thing I am concern is the bus factor of ec4rs, there is only @TheDaemoness as the only maintainer that have edit access behind it, in case anything happens in the future we need to have a fork, not a deal breaker I guess. Maybe put it within helix-editor but the coding style seemed different than usual rust projects?
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 it's important that an editorconfig library exists in the ecosystem that's decoupled from any larger codebase. I certainly don't need to be the only maintainer, but I want the library to be useful for code editors other than Helix.
That was my motivation for making ec4rs its own thing rather than just making a much larger PR.
The coding style is a bit different than usual, I'll admit that. I have some strong opinions on coding style that I'm willing to forego if there are concerns about making the project more accessible for future maintainers.
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.
Can you please add rust-bus or maybe someone else as a separate maintainer so that there is a chance to revive the project in case anything happens? But yeah, there can always be a fork. But I guess not much of a concern here.