Initialize .editorconfig editor to support syntax coloring. #38

Merged
merged 1 commit into from Jun 7, 2016

Projects

None yet

3 participants

@angelozerr
Contributor
angelozerr commented May 30, 2016 edited

This (big) PR provides

  • a custom EditorConfigEditor which support syntax coloring:

editorconfigsyntaxcolorizing

  • the syntax coloring can be customized with preferences:

editorconfigsyntaxcoloringpreferences

  • the editor supports dark theme too:

editorconfigsyntaxcoloringdark

I'm waiting for that you accept this PR to improve again with other features (completion, validation, folding, bracket, etc)

@angelozerr
Contributor

@ncjones could you review my PR please. I would like to do other PRs. Thanks!

@ncjones
Owner
ncjones commented Jun 6, 2016

@angelozerr sorry I have been really busy the last week and haven't had time to look at this. Here's some comments based on a quick look:

  1. you don't need to assign copyright to me for your own work
  2. rename the package "org.eclipse.editorconfig.internal.ui" to "org.eclipse.editorconfig.ui.internal" so that it unambiguously indicates the project it belongs to.
  3. there's no test automation. Do you have any ideas how to automate tests this?
@angelozerr
Contributor

you don't need to assign copyright to me for your own work

Are you sure with that? When I receive contributions from MyEclipse, RedHat contributions for tern.java, angularjs-eclipse, they keep the header with my name and add their name.

rename the package "org.eclipse.editorconfig.internal.ui" to "org.eclipse.editorconfig.ui.internal" so that it unambiguously indicates the project it belongs to.

All eclipse packages are like this. Ex :

  • org.eclipse.core.internal.resources.File
  • org.eclipse.core.resources.IFile

there's no test automation. Do you have any ideas how to automate tests this?

Never done and never seen tests for editor. Sorry -(

@ncjones
Owner
ncjones commented Jun 7, 2016

The package prefix needs to be unique for the jar file. In the example you gave, "org.eclipse.core.internal.resources" and "org.eclipse.core.resources" should be packages in the "org.eclipse.core" jar file. If not then this is a regrettable mistake and should not be emulated.

@angelozerr
Contributor

The package prefix needs to be unique for the jar file. In the example you gave, "org.eclipse.core.internal.resources" and "org.eclipse.core.resources" should be packages in the "org.eclipse.core" jar file. If not then this is a regrettable mistake and should not be emulated.

In this sample the JAR is or.eclipse.core.resources_.jar and not or.eclipse.core_.jar

@mickaelistria could give us your comment please about this PR for the 3) comments that @ncjones has done.

I would like really that @ncjones accepts this PR to improve again (a lot) the editor and the apply of preferences.

Thanks!

@mickaelistria

you don't need to assign copyright to me for your own work

Right, if @ncjones didn't work at all on that file, there is no need to add him as copyright owner. The grain of the copyright is the file, not the proect. So for this change, as I understand only @angelozerr wrote code on those files, only his name should be mentioned.

rename the package "org.eclipse.editorconfig.internal.ui" to "org.eclipse.editorconfig.ui.internal" so that it unambiguously indicates the project it belongs to.

I agree this is a better practice.

there's no test automation. Do you have any ideas how to automate tests this?

IMO, the simplest would be to open the file in editor, and then try to retrieve that various setttings from the editor and/or its underlying StyledText widget and check whether they match the expectation of the .editorconfig file.

@angelozerr
Contributor

Thanks @mickaelistria

@ncjones I have fixed 1) and 2).

For the tests I think tests with EditorConfigDocumentProvider could be interesting but to be honnest with you, it's a copy/paste from PropertiesFilesdEditor from JDT with clean, so I think there are not a lot of bugs.

It's difficult for me to find time to do that. Is it possible to accept this PR please. I will try to do a new PR with tests. Thanks!

@ncjones ncjones merged commit d258f6c into ncjones:master Jun 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@angelozerr
Contributor
angelozerr commented Jun 7, 2016 edited

Thanks a lot @ncjones!

Is it possible to give me rights for the wiki https://github.com/ncjones/editorconfig-eclipse/wiki, because I would like to add screenshots

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment