-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[Proposal] Extensions to file_header_template
in EditorConfig
#64177
Comments
This proposal is just a starting point. It can be extended to support the following asks. |
Because these values are generally set once and not changed. It's not a high priority area to invest in given the wealth of other scenarios that people care about and use far more often. |
Now I read it again, I see how it came off rude. For that I'm sorry. I didn't mean that way. File headers are usually multi-line and in EditorConfig, you can't set multi-line values. So, this should be the first thing you could've thought about while implementing this. I don't know why you chose not to.
The header text file and its path can be set once and don't need to be changed! This is not a valid reason.
What other scenarios do we have with a simple file header? The only scenario I could think of is adding a header with variable substitution which you already started with |
We actually did consider it, and there are various reasons why that might be better in some scenarios. The main problem is it's very difficult to get many teams to accept "another configuration file" checked into source control (and don't underestimate what I mean by "very difficult" here). We already had general acceptance of .editorconfig, so we devised a strategy to minimize additional requirements for adoption of the new feature. |
Sure, I've had experience with that line of thought. But when it does come down to it, you need to be able to look at the text as a whole and update when needed. With the inline approach, say, when the sources embed full license file, then "they" might not be happy, having to update two places at once. Since the frequency of updating the text is not a short one, people might forget that they need to be updated in two places at once. Yeah, I've had legal disputes because of this. It didn't end well. |
Also, I'm not asking to remove the existing way of applying the header. See the issue title, it says Extensions. That's right. These features only get enabled when the |
You can. They just need to be encoded in some fashion.
We did think about it. But it would be a lot of effort for minimal value. As I mentioned above, this might be a value set once for the lifetime of a project. I'm not a fan of spending a lot of effort to design something so large and complex that people would use once and then not again. |
Let me rephrase that: you can't set multi-line values in multiple lines
I'm not either but having a file path and processing the contents similarly as you would without it, is not complex at all. The proposal may have a lot of other things but the main ask is to allow path to a file header text. |
It's definitely complex. We now have to have a model for loading contents of files that may or may not exist within a project. This would require some mechanism to do this IO in the first place, something i'm not a fan of getting into. Up to now, .editorconfig files are entirely self-contained and clear on their semantics. This deeply changes this by introducing a concept of multiple files that have a templating relationship. Say i'm an analyzer, and i want to read the value of the file-header, how do i do it? Does hte analyzer need to figure out all the rules and how to load these other files and merge all these values together? Does the analyzer do IO (which is verboten), or is this handled at a different layer? If another layer, which layer would that be? It has to be at least at the compiler layer so that these instantiated values are available to the analyzers just running htere. This is not simple and def would need a lot of thought and design to just solve a problem that is trivially solvable just by encoding newlines into the final computed value. |
It changes nothing if only path to a file is implemented. If it's a file path relative to the EditorConfig file and exists, you get the contents, then proceed as you have before.
Sure, For other features, it depends on the demand from the community and the team's willingness to pursue it. The proposed design might evolve in the mean time. But pointing the |
This cannot be done at parsing time. Such code might not even live in Roslyn, it might live in entirely different components. The components also do not expose the set of files that were used in parsing (remember, editor config aggregates many values over many files in a directory tree). This is why this is not simple. |
Then how and where does Roslyn get the EditorConfig values? |
I recommend looking in Roslyn for 'AnalyzerConfig' to get a sense of the complexity involved here. As an example, Roslyn may not even be involved in parsing and merging the editorconfig files. That may be handled by VS platform itself, and we just wrap the values. Or the compiler may do this. These components are not going to add any special processing of this key/value as it will have no meaning to them. They then provide three key/values to us. But there is no concept of what file they came from. So doing a relative file read would not be possible. |
With response to this #33012 (comment):
I don't know why the team didn't think to have a file path to
file_header_template
that could contain all sorts of styled templates with or without custom comment variables or prefixes.For Example:
An example of
file-header.txt
having a simple commentbecomes...
and with multi-line options
becomes...
An example of
file-header.xml
having an XML Doc commentbecomes...
and with multi-line options
becomes...
These are some of the things that I came up with just now reading the proposal and issues people have faced with source file headers. This proposal covers almost all of the things that people have been asking in the EditorConfig repo and in issue #33012.
The text was updated successfully, but these errors were encountered: