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

Provide .editorconfig support #877

Open
angelozerr opened this issue Sep 15, 2022 · 38 comments
Open

Provide .editorconfig support #877

angelozerr opened this issue Sep 15, 2022 · 38 comments

Comments

@angelozerr
Copy link
Contributor

Today .editorconfig is supported directly in vscode, IJ, etc

It should be really nice that Eclipse IDE provides this support too. It exists a discussion at https://bugs.eclipse.org/bugs/show_bug.cgi?id=457046

And I implemented a long time ago https://github.com/angelozerr/ec4e which works with genereic editor but cannot apply all settings.

@laeubi
Copy link
Contributor

laeubi commented Sep 15, 2022

@angelozerr do you think you can prepare a PR for this to contribute the code to eclipse? I'll then open a CQ for this,

@mickaelistria
Copy link
Contributor

The edition part seems to require TM4E. Do we want Platform to start depending on TM4E ? I personally wouldn't mind, but I think it should be discussed a bit more broadly.
If we don't want it, then things like langauge configuration and highlighting would have to be removed for the moment.

@jukzi
Copy link
Contributor

jukzi commented Sep 15, 2022

some questions:
a) What happens when the .editorconfig contradicts the workspace settings?
b) What happens when the .editorconfig in the project folder contradicts the project settings?
c) When the projects setting are edited via UI will the .editorconfig reflect the change?
d) does "searching" for the .editorconfig use file system or eclipse workspace api?

@angelozerr
Copy link
Contributor Author

angelozerr commented Sep 15, 2022

At first I would like to say that ec4e is based on https://github.com/ec4j/ec4j an editorconfig parser that I have implemented which can provide the required information that we need for IDE (AST offsets of section, fault tolerant parser,etc) compared to https://github.com/editorconfig/editorconfig-core-java which don't give the capability to give some offsets.

The official site https://editorconfig.org/ references today the ec4j.

The ec4e should work well with Generic Editor but not correctly with old editor. Here a demo where you can see wizard in action, codemining to show number of files which matche the section, syntax validation of .editorconfig and the most important apply of setting for matched files:

EditorConfigDemo

The edition part seems to require TM4E. Do we want Platform to start depending on TM4E ? I personally wouldn't mind, but I think it should be discussed a bit more broadly.

Indeed, perhaps TM4E should take care of that?

a) What happens when the .editorconfig contradicts the workspace settings?

It is the .editorconfig which should have the priority. But any improvement are possible.

b) What happens when the .editorconfig in the project folder contradicts the project settings?

Again, it is the .editorconfig which should have the priority. But any improvement are possible.

c) When the projects setting are edited via UI will the .editorconfig reflect the change?

No.

d) does "searching" for the .editorconfig use file system or eclipse workspace api?

ec4j provide some abstract resource class to retrieve .editorconfig. ec4e implement them with Eclipse API IResource https://github.com/angelozerr/ec4e/tree/master/org.eclipse.ec4e/src/main/java/org/eclipse/ec4e/internal/resource

@merks
Copy link
Contributor

merks commented Sep 15, 2022

I'm not sure it's a good idea to have more circular dependencies in the platform via TM4E. Ssuch an addition would force Java 17 BREE into the platform, which is yet another issue to consider carefully.

@laeubi
Copy link
Contributor

laeubi commented Sep 16, 2022

Ssuch an addition would force Java 17 BREE into the platform, which is yet another issue to consider carefully.

Build is already Java 17, EPP is already Java 17, so having one bundle (we don't need to put the code into an exiting plugin) would not change much?

I'm not sure it's a good idea to have more circular dependencies in the platform via TM4E

@mickaelistria already mentioned one might be able to not use TM4E beside that, if we just have an org.eclipse.text.editorconfig bundle it should not be much of an issue...

@akurtakov
Copy link
Member

akurtakov commented Sep 16, 2022

Having circular dependency between tm4e and platform.releng build is a total no-go. Java 17 is not an issue for me (it's probably time to admit that Java 11 support other than producing Java 11 bytecode is of zero interest for my company team now).

@laeubi
Copy link
Contributor

laeubi commented Sep 16, 2022

Having circular dependency between tm4e and platform.releng build is a total no-go

Maybe we have a different understanding of a "circular dependency" but for me a circular dependency is where A > B > A what can not be resolved, while here we have A > C < B what can be resolved...

Anyways @angelozerr can you explain where TM4E is used and if it can be either optional or replaced with other techniques?

@akurtakov
Copy link
Member

Having circular dependency between tm4e and platform.releng build is a total no-go

Maybe we have a different understanding of a "circular dependency" but for me a circular dependency is where A > B > A what can not be resolved, while here we have A > C < B what can be resolved...

It's a releng "circular dependency" between p2 repos - platform -> tm4e -> platform

Anyways @angelozerr can you explain where TM4E is used and if it can be either optional or replaced with other techniques?

@vogella
Copy link
Contributor

vogella commented Sep 16, 2022

We have some circular dependencies already IIRC. The EMF edit plug-in IIRC.

@laeubi
Copy link
Contributor

laeubi commented Sep 16, 2022

We have some circular dependencies already IIRC. The EMF edit plug-in IIRC.

The whole platform build is a "circular dependency" ;-)

Beside that, no one forces us to use the latest snapshot repository of TM4E ... we can simply use the latest release of TM4E, of course then might only use features of TM4E - 1 but this should not be an issue here...

@akurtakov
Copy link
Member

Please, the fact that we have many issues doesn't mean we should add more! Let's agree to not make platform build worse than its current terrible state.

@akurtakov
Copy link
Member

So to move the discussion in the right direction - TM4E is used for the editor of .editorconfig files which means that applying/supporting existing .editorconfig files can go in without the requirement on TM4E and I propose the discussion to go towards that goal. Having smart editor for .editorconfig files can stay separate.

@laeubi
Copy link
Contributor

laeubi commented Sep 16, 2022

Even though I don't know how it works, but Eclipse sometimes recommend me to install things from the marketplace for "unknown" extensions.

So if we can integrate the .editorconfig itself, and link it to a market-place entry then it could be installed afterwards by a seperate project that support the editing of files.... @angelozerr would that be feasible for you?

@akurtakov
Copy link
Member

Marketplace entry that has a tag fileextension_editorconfig should be everything needed for MPC to suggest installing the editor.

@angelozerr
Copy link
Contributor Author

Dont worry I can remove TM4E which provides syntax coloration and some features with langage configuration wich provides machine bracket for .editorconfig file but the other factures like validation codemining complerion for .editorconfig is not linked to TM4E.

The only required dépendance is ec4j which is on maven central.

Keep in mind the apply of .editorconfig works correctly with only generic editor and not with some editor like jdt java editor.

More I need to use java reflection to add a custom IPreferenceStore to generic editor which is not very clean.

To be clean generic editor should provide perhaps an extension point to add an ipreferencestore

@angelozerr
Copy link
Contributor Author

For .editorconfig syntax coloration I think TM4E should provide the textmate for .editorconfig file.

@angelozerr
Copy link
Contributor Author

I have just one question for ec4j dependency, can I create a PR with plusieurs which have à lib folder which contains ec4j jar?

We could manage that properly in an another PR? What do you think about that?

@akurtakov
Copy link
Member

It's fine to have first draft with ec4j being internal dependency. Before accepting we should have it added to the target platform referring to the maven artifact directly.

@laeubi
Copy link
Contributor

laeubi commented Sep 16, 2022

I have just one question for ec4j dependency, can I create a PR with plusieurs which have à lib folder which contains ec4j jar?

You can add maven dependencies directly to the target and use is like any regular bundle, so no need for workaround like lib folders!

angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 16, 2022
Fixes #87

Signed-off-by: azerr <[email protected]>
angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 16, 2022
Fixes #87

Signed-off-by: azerr <[email protected]>
angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 17, 2022
angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 17, 2022
@angelozerr
Copy link
Contributor Author

I have created a draft PR at eclipse-platform/eclipse.platform.text#89 without TM4E but we need to do some PR before, see eclipse-platform/eclipse.platform.text#89 (comment)

@p-bakker
Copy link

p-bakker commented Sep 18, 2022

Imho having .editorconfig support consists of several things.

The first level is parsing the file and applying the correct settings from it to the editor.

The next step is acting on those preferences: while I think the indent_style and indent_size do map on existing, workspace-level settings (that are currently already taken into account in most of not all editors), settings like trim_trailing_whitespace and insert_final_newline do not (to my knowledge). These settings exist for the JDT Java editor, but aren't generic, workspace-wide settings available to/implemented in all editors.

I for one would expect that if Eclipse would support .editorconfig, it would also mean that all the settings one can set via .editorconfig would also be supported in all editors (although personally I'd be ok with just support in the generic editor)

Third level would be rich editing support for .editorconfig files

@p-bakker
Copy link

some questions:
a) What happens when the .editorconfig contradicts the workspace settings?
b) What happens when the .editorconfig in the project folder contradicts the project settings?
c) When the projects setting are edited via UI will the .editorconfig reflect the change?

Per definition, .editorconfig overrides workspace- and project-level settings: it defines settings on a per-fileType basis, looking for the closets applicable settings by looking for the closets .editorconfig file (containing relevant settings) from the current filled location up through its parent directory structure, until either matching settings have been found or a .editorconfig file is encountered that has root = true

@angelozerr
Copy link
Contributor Author

I for one would expect that if Eclipse would support .editorconfig, it would also mean that all the settings one can set via .editorconfig would also be supported in all editors (although personally I'd be ok with just support in the generic editor)

Given that Eclipse is extensible "all" might never be achievable ... so starting with "one" that is recommended to be used for new editors seems suitable to me.

It is an important question, should .editorconfig works with any editor or with just genereic editor? The answer will determine where the extension point which create and add an IPreferenceStore into the editor (in texteditor extension point or in genereric extension point).

@laeubi
Copy link
Contributor

laeubi commented Sep 18, 2022

Just assume I have an editor that supports Grahical editing of a binary file format, has "trim_trailing_whitespace" any meaning?

@laeubi
Copy link
Contributor

laeubi commented Sep 18, 2022

By the way, as far as I know no one prevents others to also looking at the generic editor extension point later on...

angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 18, 2022
angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 18, 2022
angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 19, 2022
@jukzi
Copy link
Contributor

jukzi commented Sep 19, 2022

Again, it is the .editorconfig which should have the priority. But any improvement are possible.

whatever takes precedence should be shown in the properties file of any resource:

  1. the effective used file
  2. the effective setting
    image

@laeubi
Copy link
Contributor

laeubi commented Sep 19, 2022

whatever takes precedence should be shown in the properties file of any resource:

I don't think that this is mandatory, any editor can ignore these defaults these are just resources settings. Still it would be useful if there would be an extension point to inform the user about that these settings are overridden somewhere else.

@angelozerr
Copy link
Contributor Author

I agree with your all comments, but to be honnest with you I could not do that in the same PR and I'm not sure that I will have time to implement all of your ideas. My main goal is to provide .editorconfig basicly and after we could improve it step by step.

angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 19, 2022
Fixes #87

Signed-off-by: azerr <[email protected]>
angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 19, 2022
Fixes #87

Signed-off-by: azerr <[email protected]>
angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 20, 2022
Fixes #87

Signed-off-by: azerr <[email protected]>
angelozerr referenced this issue in angelozerr/eclipse.platform.text Sep 20, 2022
Fixes #87

Signed-off-by: azerr <[email protected]>
@angelozerr
Copy link
Contributor Author

To provide a clean API to contribute to generic editor with custom preferences store (with extension point), I need thoses 2 PRs:

@laeubi laeubi transferred this issue from eclipse-platform/eclipse.platform.text Jun 28, 2023
@mickaelistria
Copy link
Contributor

I don't think Eclipse Platform will ever provide ECJ support directly, but instead would expect it to be provided by another plugin from another project. So I'm closing the issue here.
But the goal is still interesting and proposals to improve ability to add support for external formatting options from 3rd party plugins are still very welcome.

@mickaelistria mickaelistria closed this as not planned Won't fix, can't repro, duplicate, stale Nov 23, 2023
@laeubi
Copy link
Contributor

laeubi commented Dec 19, 2024

@angelozerr are you still interested in bringing editorconfig support to eclipse?

I just got hit by the problem that an project uses editorconfig and it interferes with my usual settings so I need to manually reformat the code (and be careful Eclipse don't break it). And as many things have changed over time (e.g. Java 17 is no issue anymore at all) I think it might make sense to pick up this again.

I also reviewed a bit the code you shared here, the PR and the github repository and I think we have different topics that can be addressed independently:

  1. Make Eclipse capable of reading .editorconfig files, this could be done for example https://github.com/angelozerr/ec4j can you give a status here what is the current state, e.g. is it still maintained, releases to maven central for example and so on? Any special dependencies we should be aware of? Or do you propose any alternative parser as of today?
  2. Add an editor for .editorconfig files, I think this is somehow part of https://github.com/angelozerr/ec4e and could stay there independently but we might want to include it into EPPs (FYI @jonahgraham )
  3. Take editor config into account for generic editor, that's also part of https://github.com/angelozerr/ec4e and would be good to be ported/included here by default (no additional UI required as one can edit the file directly)
  4. Take editorconfig into account for JDT: Here my idea would be to either have a gobal preference "Take editorconfig into account" on the formater settings, or have a setting in the formater profile "allow override by editorconfig" so it reads some settings from there and overrides the ones specified in the profile, most notable the used whitespace character could be a very first and basic step
  5. Take encoding settings into account (as mentioned by @jukzi previously) this is independent from editors and/or formatter and one seem to plug into the filetypes here that already can provide an encoding for different filetypes (but in a static way).

@laeubi laeubi reopened this Dec 19, 2024
laeubi added a commit to laeubi/eclipse.jdt.ui that referenced this issue Dec 19, 2024
This is a very basic proof-of-concept for a way to support editor config
to make java formatter integrate with this so called cross-ide way to
specify formatter settings.

See eclipse-platform/eclipse.platform.ui#877
@laeubi
Copy link
Contributor

laeubi commented Dec 19, 2024

I now added a very basic proof-of-concept that already works very well in my setup that simply overrides the tab char:

@angelozerr
Copy link
Contributor Author

@angelozerr are you still interested in bringing editorconfig support to eclipse?

Even if the topic is super interesting, unfortunately I will have no time to do that again -(

Make Eclipse capable of reading .editorconfig files, this could be done for example https://github.com/angelozerr/ec4j can you give a status here what is the current state, e.g. is it still maintained, releases to maven central for example and so on? Any special dependencies we should be aware of? Or do you propose any alternative parser as of today?

https://github.com/angelozerr/ec4j is my repository I used when I have created this project. I have created this project because the only official project which .editorconfig parser in Java was https://github.com/editorconfig/editorconfig-core-java#readme was not
helpfull to integrate it in an IDE, because it doesn't take care of AST node.

It is the one reason why I have created this project and today the official project is https://github.com/ec4j/ec4j The very good news is that this parser is today mature and robust:

Add an editor for .editorconfig files, I think this is somehow part of https://github.com/angelozerr/ec4e and could stay there independently but we might want to include it into EPPs (FYI @jonahgraham )

This project is based on https://github.com/angelozerr/ec4j and uses

  • tm4e for supporting syntax coloration -> platform UI cannot add this dependency to tm4e, I think tm4e should include the textmate grammar from .editorconfig
  • generic editor to add codelens, validation, hover, completion -> those all code could be hosted in the platform

But the most important thing is apply the editorconfig properties to a given editor (for standard editor and generic editor both). It is the hardest topic. I spend a lot of time to find a generic solution (used by generic editor orother component), And I think the first topic that you will need to work is eclipse-platform/eclipse.platform.text#130

Take editor config into account for generic editor, that's also part of https://github.com/angelozerr/ec4e and would be good to be ported/included here by default (no additional UI required as one can edit the file directly)

It is the easy part and you could copy/paste ec4e inside Platform UI (without the textmate grammar)

Take editorconfig into account for JDT: Here my idea would be to either have a gobal preference "Take editorconfig into account" on the formater settings, or have a setting in the formater profile "allow override by editorconfig" so it reads some settings from there and overrides the ones specified in the profile, most notable the used whitespace character could be a very first and basic step

The first job to do is to provide an extension point used by the AbstractTextEditor eclipse-platform/eclipse.platform.text#130

It is long and hard work, thanks so much @laeubi to work on this great features again!

@angelozerr
Copy link
Contributor Author

angelozerr commented Dec 19, 2024

@laeubi I think the full PR that you should see is https://github.com/eclipse-platform/eclipse.platform.text/pull/109/files but it is working only with generic editor if I remember

@angelozerr
Copy link
Contributor Author

@laeubi about the editorconfig editor, you can see all features that I had implemented in https://github.com/angelozerr/ec4e/tree/master/org.eclipse.ec4e/src/main/java/org/eclipse/ec4e/internal (
codemining, completion, folding, hover , outline, validation and wizards)

Those feature uses the ec4j parser which build an AST nodes. codemining support is very nice because it displays codeming before section and you can click on it to perform the Search UI to retrieve all files which matches the section.

@angelozerr
Copy link
Contributor Author

ec4j is available on maven central https://mvnrepository.com/artifact/org.ec4j you will need the ec4j core https://github.com/ec4j/ec4j

The ec4j parser isvery robust because it uses the official test of editorconfig https://github.com/ec4j/ec4j/tree/master/core
and we have a lot of test for the parser https://github.com/ec4j/ec4j/tree/master/core/src/test/java/org/ec4j/core

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 a pull request may close this issue.

8 participants