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

[core] Add operations to manipulate a document #828

Merged
merged 7 commits into from
Jan 25, 2018
Expand Up @@ -7,14 +7,12 @@
import java.io.BufferedReader;
import java.io.Closeable;
import java.io.File;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.io.Writer;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardCopyOption;
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;
Expand All @@ -36,26 +34,42 @@ public class DocumentFile implements Document, Closeable {
private final BufferedReader reader;
private int currentPosition = 0;

private final Path temporaryPath = Files.createTempFile("pmd-", null);
private final Writer writer = new FileWriter(temporaryPath.toFile());
private final Path temporaryPath = Files.createTempFile("pmd-", ".tmp");
private final Writer writer = Files.newBufferedWriter(temporaryPath, StandardCharsets.UTF_8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the charset assumption may be wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anywhere I can get the charset for the file i'm editing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PMD itself takes it from the user input (CLI, Gradle, Maven, etc.), we should probably honor that. Just have both encodings be the same, and taken at the constructor, so you can hook it to the current execution configuration at the proper layer.


public DocumentFile(final File file) throws IOException {
reader = new BufferedReader(new FileReader(file));
reader = Files.newBufferedReader(file.toPath(), StandardCharsets.UTF_8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the charset assumption may be wrong

this.filePath = file.getAbsolutePath();
mapLinesToOffsets();
}

private void mapLinesToOffsets() throws IOException {
try (Scanner scanner = new Scanner(filePath)) {
try (Scanner scanner = new Scanner(Paths.get(filePath))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not have filePath be a Path directly?

int currentGlobalOffset = 0;

while (scanner.hasNextLine()) {
lineToOffset.add(currentGlobalOffset);
currentGlobalOffset += scanner.nextLine().length();
currentGlobalOffset += getLineLengthWithLineSeparator(scanner);
}
}
}

/**
* Sums the line length without the line separation and the characters which matched the line separation pattern
* @param scanner the scanner from which to read the line's length
* @return the length of the line with the line separator.
*/
private int getLineLengthWithLineSeparator(final Scanner scanner) {
int lineLength = scanner.nextLine().length();
final String lineSeparationMatch = scanner.match().group(1);

if (lineSeparationMatch != null) {
lineLength += lineSeparationMatch.length();
}

return lineLength;
}

@Override
public void insert(int beginLine, int beginColumn, final String textToInsert) {
try {
Expand Down Expand Up @@ -135,7 +149,10 @@ public void close() throws IOException {
}
reader.close();
writer.close();
Files.copy(temporaryPath, Paths.get(filePath), StandardCopyOption.REPLACE_EXISTING);

if (!temporaryPath.toFile().renameTo(new File(filePath))) {
throw new IOException("Fixed file could not be renamed. Original path = " + filePath);
}
temporaryPath.toFile().delete();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do this copy + delete rather than simply rename?

}

Expand All @@ -146,4 +163,8 @@ private void writeUntilEOF() throws IOException {
writer.write(line);
}
}

public List<Integer> getLineToOffset() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why public? at most it should be package-private if this is test-only

return lineToOffset;
}
}
Expand Up @@ -12,15 +12,15 @@

public class DocumentOperationsApplierForNonOverlappingRegions {

private static final Comparator<DocumentOperation> COMPARATOR = new DocumentOperationNonOverlappingRegionsComparator();

private final Document document;
private final List<DocumentOperation> operations;
private final Comparator<DocumentOperation> comparator;

private boolean applied;

public DocumentOperationsApplierForNonOverlappingRegions(final Document document) {
this.document = Objects.requireNonNull(document);
comparator = new DocumentOperationNonOverlappingRegionsComparator();
operations = new ArrayList<>();
applied = false;
}
Expand All @@ -39,7 +39,7 @@ private void assertOperationsHaveNotBeenApplied() {
}

private int getIndexForDocumentOperation(final DocumentOperation documentOperation) {
int potentialIndex = Collections.binarySearch(operations, documentOperation, comparator);
int potentialIndex = Collections.binarySearch(operations, documentOperation, COMPARATOR);

if (potentialIndex < 0) {
return ~potentialIndex;
Expand All @@ -53,7 +53,7 @@ private int getIndexForDocumentOperation(final DocumentOperation documentOperati
}

private boolean areSiblingsEqual(final int index) {
return comparator.compare(operations.get(index), operations.get(index + 1)) == 0;
return COMPARATOR.compare(operations.get(index), operations.get(index + 1)) == 0;
}

public void apply() {
Expand All @@ -65,15 +65,15 @@ public void apply() {
}
}

private class DocumentOperationNonOverlappingRegionsComparator implements Comparator<DocumentOperation> {
private static class DocumentOperationNonOverlappingRegionsComparator implements Comparator<DocumentOperation> {

@Override
public int compare(final DocumentOperation o1, final DocumentOperation o2) {
final RegionByLine r1 = Objects.requireNonNull(o1).getRegionByLine();
final RegionByLine r2 = Objects.requireNonNull(o2).getRegionByLine();

final int comparison;
if (areInsertOperations(r1, r2)) {
if (areInsertOperationsAtTheSameOffset(r1, r2)) {
comparison = 0;
} else if (doesFirstRegionEndBeforeSecondRegionBegins(r1, r2)) {
comparison = -1;
Expand All @@ -85,7 +85,7 @@ public int compare(final DocumentOperation o1, final DocumentOperation o2) {
return comparison;
}

private boolean areInsertOperations(final RegionByLine r1, final RegionByLine r2) {
private boolean areInsertOperationsAtTheSameOffset(final RegionByLine r1, final RegionByLine r2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we keep not knowing (from these classes perspective) if these are insert operations. You either check the operation type, or you drop that from the name.

return r1.getBeginLine() == r2.getBeginLine() && r1.getBeginColumn() == r2.getBeginColumn()
&& r1.getBeginLine() == r1.getEndLine() && r1.getBeginColumn() == r1.getEndColumn();
}
Expand Down
Expand Up @@ -6,22 +6,21 @@

import static org.junit.Assert.assertEquals;

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileWriter;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;





public class DocumentFileTest {

private static final String FILE_PATH = "psvm.java";
Expand All @@ -35,11 +34,6 @@ public void setUpTemporaryFiles() throws IOException {
temporaryFile = temporaryFolder.newFile(FILE_PATH);
}

@After
public void deleteTemporaryFiles() {
temporaryFile.delete();
}

@Test
public void insertAtStartOfTheFileShouldSucceed() throws IOException {
writeContentToTemporaryFile("static void main(String[] args) {}");
Expand Down Expand Up @@ -218,8 +212,59 @@ public void insertDeleteAndReplaceVariousTokensShouldSucceed() throws IOExceptio
}
}

@Test
public void lineToOffsetMappingWithLineFeedShouldSucceed() throws IOException {
final String code = "public static int main(String[] args) {" + '\n'
+ "int var;" + '\n'
+ "}";
writeContentToTemporaryFile(code);

final List<Integer> expectedLineToOffset = new ArrayList<>();
expectedLineToOffset.add(0);
expectedLineToOffset.add(40);
expectedLineToOffset.add(49);

try (DocumentFile documentFile = new DocumentFile(temporaryFile)) {
assertEquals(expectedLineToOffset, documentFile.getLineToOffset());
}
}

@Test
public void lineToOffsetMappingWithCarriageReturnFeedLineFeedShouldSucceed() throws IOException {
final String code = "public static int main(String[] args) {" + "\r\n"
+ "int var;" + "\r\n"
+ "}";
writeContentToTemporaryFile(code);

final List<Integer> expectedLineToOffset = new ArrayList<>();
expectedLineToOffset.add(0);
expectedLineToOffset.add(41);
expectedLineToOffset.add(51);

try (DocumentFile documentFile = new DocumentFile(temporaryFile)) {
assertEquals(expectedLineToOffset, documentFile.getLineToOffset());
}
}

@Test
public void lineToOffsetMappingWithMixedLineSeparatorsShouldSucceed() throws IOException {
final String code = "public static int main(String[] args) {" + "\r\n"
+ "int var;" + "\n"
+ "}";
writeContentToTemporaryFile(code);

final List<Integer> expectedLineToOffset = new ArrayList<>();
expectedLineToOffset.add(0);
expectedLineToOffset.add(41);
expectedLineToOffset.add(50);

try (DocumentFile documentFile = new DocumentFile(temporaryFile)) {
assertEquals(expectedLineToOffset, documentFile.getLineToOffset());
}
}

private void writeContentToTemporaryFile(final String content) throws IOException {
try (FileWriter writer = new FileWriter(temporaryFile)) {
try (BufferedWriter writer = Files.newBufferedWriter(temporaryFile.toPath(), StandardCharsets.UTF_8)) {
writer.write(content);
}
}
Expand Down
Expand Up @@ -15,7 +15,6 @@
import java.util.LinkedList;
import java.util.List;

import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -36,11 +35,6 @@ public void setUpTemporaryFiles() throws IOException {
temporaryFile = temporaryFolder.newFile(FILE_PATH);
}

@After
public void deleteTemporaryFiles() {
temporaryFile.delete();
}

@Test
public void insertAtStartOfTheDocumentShouldSucceed() throws IOException {
writeContentToTemporaryFile("static void main(String[] args) {}");
Expand Down