Add capability to get properties by JacksonBailey · Pull Request #42 · ncjones/editorconfig-eclipse

JacksonBailey · 2017-02-15T16:16:48Z

@ncjones Please don't blindly merge this because I haven't ran added functional tests or installed and tested locally yet, I just wanted your initial thoughts.

Also, I can't get the functional tests to work locally (even without my changes), can they not be ran from Windows? When I run mvn -f editorconfig-eclipse-functional-test/pom.xml integration-test it opens an Eclipse window and just hangs for a while and then prints out a lot of tests failing with this output

Tests run: 8, Failures: 0, Errors: 8, Skipped: 0, Time elapsed: 45.319 sec <<< FAILURE! - in com.ncjones.editorconfig.eclipse.test.EditorConfigTest
testIndentStyleTab[test.xml](com.ncjones.editorconfig.eclipse.test.EditorConfigTest)  Time elapsed: 5.135 sec  <<< ERROR!
java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/code/external-projects/editorconfig-eclipse/editorconfig-eclipse-functional-test/target/work/data/\editorconfig-test\.project
        at sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
        at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
        at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
        at sun.nio.fs.WindowsPath.parse(WindowsPath.java:94)
        at sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:255)
        at java.nio.file.Paths.get(Paths.java:84)
        at com.ncjones.editorconfig.eclipse.test.EditorConfigTestContext.createJavaProjectIfNotExists(EditorConfigTestContext.java:68)
        at com.ncjones.editorconfig.eclipse.test.EditorConfigTestContext.<init>(EditorConfigTestContext.java:45)
        at com.ncjones.editorconfig.eclipse.test.EditorConfigTest.setUp(EditorConfigTest.java:55)

- See: #41

This is a workaround for some of the editors needing more than a single property during visiting. - See: #41

The XML editor has two properties, `indentationChar` and `indentationSize`. `indentationChar` is which character to use for indentation (as expected) but `indentationSize` is how many of the *`indentationChar` character* to use, now *how many columns one level of indentation should be.* As such, If your `.editorconfig` file has `indent_style = tab` and `indent_size = 2` (something like below for example) then the XML editor will use *2 tabs* for one level of indentation. [*] indent_size = 2 [*.xml] indent_style = tab Each of those 2 tabs will be 2 columns wide so the individual tabs are being *displayed* correctly, just not *placed* correctly. This change makes it so that the XML editor's `indentationSize` set to the `.editorconfig` file's `indent_size`'s value *only* when the `indent_style` is `space`. When it is `tab` the XML editor's `indentationSize` is cleared. - See: #41

JacksonBailey · 2017-02-15T17:03:52Z

....editorconfig.ui/src/main/java/org/eclipse/editorconfig/ui/internal/EditorConfigVisitor.java

@@ -0,0 +1,99 @@
+/*
+ * Copyright 2017 Nathan Jones, Jackson Bailey


@ncjones I put your name here even though you haven't touched this file since all the code was copied and pasted from a file you wrote.

The tests have never been run on Windows. I have tried to fix the platform-dependent nature of the test code in the "cross-platform-functional-tests" branch by using Paths.get(URI) instead of Paths.get(String[]). Can you please pull that branch and see if it works for you now?

That fixes it, they work with and without my changes now. If want to you merge those into master I can rebase or if you don't like rebases I can just cherry-pick it.

I've merged my fixes for the functional test suite on Windows. You can rebase this branch on master.

ncjones · 2017-02-15T21:18:49Z

org.eclipse.editorconfig.core/src/main/java/org/eclipse/editorconfig/core/EditorFileConfig.java

 		return configProperties;
 	}

+	public ConfigProperty getConfigProperty(String name) {


Please write a unit test for this method.

ncjones · 2017-02-15T21:21:03Z

....editorconfig.ui/src/main/java/org/eclipse/editorconfig/ui/internal/EditorConfigVisitor.java

+	@Override
+	public void visitIndentSize(final ConfigProperty<Integer> property) {
+		final String indentSizeString = property.getValue().toString();
+		final Object indentStyle = editorFileConfig.getConfigProperty("INDENT_STYLE").getValue();


Can getConfigProperty("INDENT_STYLE") ever return null?

Yes, it looks like if indent style is not set in the file or any parent then it could return null.

I added a null-check to avoid exceptions. indentStyle can still be null but it shouldn't matter.

JacksonBailey · 2017-04-20T20:20:18Z

Sorry for disappearing for a couple of months on this. I've merged master into my branch and the (current) functional tests all pass. I will add some for the new features as well as unit tests and I will check if getConfigProperty("INDENT_STYLE") can return null or not.

JacksonBailey · 2017-04-20T20:22:48Z

editorconfig-eclipse-functional-test/META-INF/MANIFEST.MF

 Bundle-Name: EditorConfig Eclipse Functional Test
 Bundle-SymbolicName: editorconfig-eclipse-functional-test;singleton:=true
-Bundle-Version: 0.3.0.qualifier
+Bundle-Version: 0.4.0.qualifier


I don't know if this needed to be changed, but it looked like here and in the POM the version wasn't bumped. I can remove this commit if it's not supposed to be here.

This was overlooked. I've already pushed this change to master and updated the "update-versions.sh" script so that this doesn't happen again.

JacksonBailey · 2017-04-20T22:49:46Z

@ncjones I was adding functional tests for the new feature but the test for Ant's build.xml file with spaces failed due to a change in the test for tabs. Your original cross platform test is still present so I checked it out and made the same change to make sure it wasn't my changes that caused this but Maven still insisted on using my newer version of the plugin; I can't say for sure it wasn't my changes currently. I don't know where Maven is getting it from. Do you know where they are so I can delete them and use the old version?

[WARNING] The following locally built units have been used to resolve project dependencies:
[WARNING]   org.eclipse.editorconfig.core/0.4.0.201704202147
[WARNING]   editorconfig-eclipse-feature.feature.jar/0.4.0.201704202147
[WARNING]   org.eclipse.editorconfig.ui/0.4.0.201704202147
[WARNING]   editorconfig-eclipse-feature.feature.group/0.4.0.201704202147

(Here is the commit I made showing the problem. If you remove the tab test or don't have the indent_size = 3 line then everything works fine. I checked and your test is correctly putting in a new .editorconfig file with the correct contents, so I'm not sure what the issue is.)

ncjones · 2017-04-21T10:26:25Z

@JacksonBailey maven artifacts are by default cached in ~/.m2. I would do something like the following in Bash to remove relevant artifacts:

find "$HOME/.m2" -type d -name '*editorconfig*' | xargs rm -rf

JacksonBailey · 2017-04-21T20:22:54Z

I checked my m2, but I was looking in org.eclipse.editorconfig.*, the name threw me off since it looked like a group name. In any case, once I cleaned it I still get the failure. I don't know if this is a quirk of the test or a real bug in the code. (Maybe it's not re-checking .editorconfig file?)

ncjones · 2017-04-26T10:18:11Z

I've cherry picked the commits for refactoring the visitor and adding the method to get properties by name. Can you please rebase the remainder of the work?

I have two concerns about the functional change here:

the indent style is only set when visiting the indent size. Doesn't this break the indent style when no indent size is specified in .editorconfig?
it seems with your testing in the "failing-functional-test-in-ant" branch you have discovered that the Ant editor has the same issues that you are trying to fix for the XML editor. Can the same fix be applied for the Ant editor settings? I think that would allow your functional test to pass.

By the way, there should be a new test for this scenario (indent size + style) instead of just adding indent size to the existing indent style test.

victornoel · 2018-06-09T17:10:53Z

This PR is superseded by #47.

JacksonBailey · 2020-02-11T16:19:29Z

Closed, please see #47

Jackson Bailey added 4 commits February 14, 2017 16:31

Add a method to get a property by name

9bd2108

- See: #41

Split visitor logic from activation handler logic

2f65dc6

Give EditorConfigVisitor entire EditorFileConfig

d736e16

This is a workaround for some of the editors needing more than a single property during visiting. - See: #41

JacksonBailey commented Feb 15, 2017

View reviewed changes

ncjones reviewed Feb 15, 2017

View reviewed changes

Jackson Bailey added 2 commits April 20, 2017 16:10

Merge branch 'master' into add-capability-to-get-properties

37a0e9d

Update version of functional test to 0.4.0

2e9bfce

JacksonBailey commented Apr 20, 2017

View reviewed changes

Jackson Bailey added 3 commits April 20, 2017 17:14

Check for nulls when getting indent style

bc5a87b

Add unit test for EditorFileConfig#getConfigProperty

0819e38

Add message to easily see which test failed

31c8d2f

ncjones mentioned this pull request Apr 26, 2017

Provide an extension point to declare binding between Eclipse editor #39

Open

ncjones mentioned this pull request Jun 4, 2018

XML Editor behaves incorrectly when using tabs and indent_size other than 1 #41

Closed

This was referenced Jun 9, 2018

Adds tab_width feature #45

Open

Fix xml editor #47

Open

JacksonBailey closed this Feb 11, 2020

		@@ -0,0 +1,99 @@
		/*
		* Copyright 2017 Nathan Jones, Jackson Bailey

Conversation

JacksonBailey commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JacksonBailey commented Apr 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JacksonBailey commented Apr 20, 2017

Uh oh!

ncjones commented Apr 21, 2017

Uh oh!

JacksonBailey commented Apr 21, 2017

Uh oh!

ncjones commented Apr 26, 2017

Uh oh!

victornoel commented Jun 9, 2018

Uh oh!

JacksonBailey commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants