-
Notifications
You must be signed in to change notification settings - Fork 49
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
Better indentation when adding GCP libraries to pom.xml #3511
base: master
Are you sure you want to change the base?
Conversation
…atform#3381 Change-Id: I5d4a22b089f1516845e8597cf11c5198772dc4dd
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'm not yet sure if this is the right approach but if it is, it needs a test.
Codecov Report
@@ Coverage Diff @@
## master #3511 +/- ##
=========================================
Coverage ? 70.10%
Complexity ? 2990
=========================================
Files ? 376
Lines ? 13632
Branches ? 1605
=========================================
Hits ? 9557
Misses ? 3432
Partials ? 643
Continue to review full report at Codecov.
|
...ine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/POMFormatTest.java
Outdated
Show resolved
Hide resolved
...ine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/POMFormatTest.java
Outdated
Show resolved
Hide resolved
...ine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/POMFormatTest.java
Outdated
Show resolved
Hide resolved
...ine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/POMFormatTest.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
private static void assertFileContentsEqual(File actual, File expected) throws IOException { | ||
assertArrayEquals(Files.readAllBytes(actual.toPath()), Files.readAllBytes(expected.toPath())); |
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.
swap arguments since expected customarily comes before actual
URL fileURL = bundle.getEntry("/testdata/formatRemove.xml"); | ||
File expected = new File(FileLocator.resolve(fileURL).toURI()); | ||
URL fileUrl = bundle.getEntry("/testdata/formatRemove.xml"); | ||
File expected = new File(FileLocator.resolve(fileUrl).toURI()); | ||
assertFileContentsEqual(pomFile.getLocation().toFile(), expected); |
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.
swap arguments since expected customarily comes before actual
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.
Tests are failing:
Tests run: 2, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.634 s <<< FAILURE! - in com.google.cloud.tools.eclipse.appengine.libraries.PomFormatTest
testAddDependencies(com.google.cloud.tools.eclipse.appengine.libraries.PomFormatTest) Time elapsed: 0.36 s <<< FAILURE!
java.lang.AssertionError: array lengths differed, expected.length=1168 actual.length=1135
at org.junit.Assert.fail(Assert.java:88)
at org.junit.internal.ComparisonCriteria.assertArraysAreSameLength(ComparisonCriteria.java:76)
at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:37)
at org.junit.Assert.internalArrayEquals(Assert.java:532)
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.
Tests are failing:
Tests run: 2, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.634 s <<< FAILURE! - in com.google.cloud.tools.eclipse.appengine.libraries.PomFormatTest
testAddDependencies(com.google.cloud.tools.eclipse.appengine.libraries.PomFormatTest) Time elapsed: 0.36 s <<< FAILURE!
java.lang.AssertionError: array lengths differed, expected.length=1168 actual.length=1135
Not failing locally. will check
@@ -482,10 +482,29 @@ private static String getValue(Element dependency, String childName) { | |||
private void writeDocument() throws CoreException, TransformerException { | |||
Transformer transformer = transformerFactory.newTransformer(); | |||
transformer.setOutputProperty(OutputKeys.INDENT, "yes"); | |||
transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount","2"); //$NON-NLS-1$ //$NON-NLS-2$ |
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.
This is the core fix. Other changes are for removing extra spaces and blank line when add or removing libaries.
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.
Sorry. Just noticed this was still out for review.
return; | ||
} | ||
|
||
final List<String> diff = new ArrayList<>(); |
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.
Google style avoids final on local variables where not required.
|
||
private void addChild(Node parent, Node newChild) { | ||
parent.appendChild(newChild); | ||
removeWhiteSpaceBefore(newChild);// for child 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.
nit: space before //
} | ||
|
||
private static void removeWhiteSpaceBefore(Node node) { | ||
Node prevElement = node.getPreviousSibling(); |
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.
previousElement
Change-Id: I5d4a22b089f1516845e8597cf11c5198772dc4dd
fixes #3381