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

FileText should not extends AbstractList #3034

Closed
romani opened this Issue Mar 13, 2016 · 8 comments

Comments

@romani
Member

romani commented Mar 13, 2016

triggered by #3028 (comment)

problem is https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/FileText.java#L57

public final class FileText extends AbstractList<String> {

FileText do override of two methods only in AbstractList and never use super, do we ever do remove as AbstractList allow ?
Investigate a bit more and remove change class to avoid extends AbstractList in checkstyle 8 version.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 26, 2016

Member

do we ever do remove as AbstractList allow ?

I did a quick test, and no. We never call remove. Whatever is currently written in FileText is all we use.
I assume we pass FileText as a reference to all filesets and checks. So calling remove will change what those checks see, and I do not see a reason for allowing this. It could potentially break Java parsing if not done correctly.

The only external place outside FileText, where this change will be an issue, is where we swap uses of List<String> and FileText.
Example:
https://github.com/checkstyle/checkstyle/tree/5530d4c27d508c513b68af67aadede84b4cfa102/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L314
Here where we send FileText but the declaration actually says List<String>.

Edit:
Since we are breaking compatibility, is there a reason we can't join FileContents and FileText?
FileContents seems to be just a special wrapper of FileText.

Member

rnveach commented Oct 26, 2016

do we ever do remove as AbstractList allow ?

I did a quick test, and no. We never call remove. Whatever is currently written in FileText is all we use.
I assume we pass FileText as a reference to all filesets and checks. So calling remove will change what those checks see, and I do not see a reason for allowing this. It could potentially break Java parsing if not done correctly.

The only external place outside FileText, where this change will be an issue, is where we swap uses of List<String> and FileText.
Example:
https://github.com/checkstyle/checkstyle/tree/5530d4c27d508c513b68af67aadede84b4cfa102/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L314
Here where we send FileText but the declaration actually says List<String>.

Edit:
Since we are breaking compatibility, is there a reason we can't join FileContents and FileText?
FileContents seems to be just a special wrapper of FileText.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 2, 2016

Member

@romani This looks like a Checkstyle 8 candidate.

Member

rnveach commented Nov 2, 2016

@romani This looks like a Checkstyle 8 candidate.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 24, 2017

Contributor

@rnveach

Since we are breaking compatibility, is there a reason we can't join FileContents and FileText?
FileContents seems to be just a special wrapper of FileText.

I don't think that it is a task for this issue. Lets fix what is mentioned in @romani's first comment, as this issue blocks #4468.

Contributor

MEZk commented Jun 24, 2017

@rnveach

Since we are breaking compatibility, is there a reason we can't join FileContents and FileText?
FileContents seems to be just a special wrapper of FileText.

I don't think that it is a task for this issue. Lets fix what is mentioned in @romani's first comment, as this issue blocks #4468.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 24, 2017

Member

If that helps, please split issue in few issues

Member

romani commented Jun 24, 2017

If that helps, please split issue in few issues

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 24, 2017

Contributor

@rnveach @romani
Split issue. #4523

Contributor

MEZk commented Jun 24, 2017

@rnveach @romani
Split issue. #4523

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jun 25, 2017

Collaborator

@MEZk @rnveach

AbstractFileSetCheck#processFiltered accepts List<String> lines, after removing extension extends AbstractList<String> it makes impossible to accept instances of FileText class.

We can implement something like FileText.getLines which will return List<String> object and pass it to the AbstractFileSetCheck#process method, but it may cause next problem

RegexpMultilineCheck implements AbstractFileSetCheck#processFiltered in the following way

 @Override
    protected void processFiltered(File file, List<String> lines) {
        detector.processLines(FileText.fromLines(file, lines));
    }

It uses static method FileText.fromLines:

public static FileText fromLines(File file, List<String> lines) {
       final FileText fileText;
       if (lines instanceof FileText) {
           fileText = (FileText) lines;
       }
       else {
           fileText = new FileText(file, lines);
       }
       return fileText;
   }

It means, if we remove extends, then FileText object will be recreated from lines of the previous FileText object, what causes to recreate object from reformatted text, what means the following test will fail

@Test
 public void testFileTextRecreation() throws IOException {
     final String charsetName = "ISO-8859-1";
     final File file = temporaryFolder.newFile();
     Files.write(file.toPath(),
             "first line \r\n second line \n\r third line".getBytes(StandardCharsets.UTF_8));
     final FileText fileText = new FileText(file, charsetName);
     FileText anotherFileText = FileText.fromLines(file, fileText.getLines());
     String one = fileText.getFullText().toString().replace("\n", "\\n").replace("\r", "\\r");
     String two = anotherFileText.getFullText().toString().replace("\n", "\\n").replace("\r", "\\r");
     assertEquals(one, two);
 }

Result is

Expected :first line \r\n second line \n\r third line
Actual   :first line \n second line \n\n third line\n
  • Possible solution is to add new method or change existing FileSetCheck.process(File file, FileText fileText)
  • Add new method or change existing AbstractFileSetCheck.processFiltered(File file, List<String> lines)
  • And implement/change all occurrences of these methods in code base

What do you think? I can PR to show the problem

Collaborator

timurt commented Jun 25, 2017

@MEZk @rnveach

AbstractFileSetCheck#processFiltered accepts List<String> lines, after removing extension extends AbstractList<String> it makes impossible to accept instances of FileText class.

We can implement something like FileText.getLines which will return List<String> object and pass it to the AbstractFileSetCheck#process method, but it may cause next problem

RegexpMultilineCheck implements AbstractFileSetCheck#processFiltered in the following way

 @Override
    protected void processFiltered(File file, List<String> lines) {
        detector.processLines(FileText.fromLines(file, lines));
    }

It uses static method FileText.fromLines:

public static FileText fromLines(File file, List<String> lines) {
       final FileText fileText;
       if (lines instanceof FileText) {
           fileText = (FileText) lines;
       }
       else {
           fileText = new FileText(file, lines);
       }
       return fileText;
   }

It means, if we remove extends, then FileText object will be recreated from lines of the previous FileText object, what causes to recreate object from reformatted text, what means the following test will fail

@Test
 public void testFileTextRecreation() throws IOException {
     final String charsetName = "ISO-8859-1";
     final File file = temporaryFolder.newFile();
     Files.write(file.toPath(),
             "first line \r\n second line \n\r third line".getBytes(StandardCharsets.UTF_8));
     final FileText fileText = new FileText(file, charsetName);
     FileText anotherFileText = FileText.fromLines(file, fileText.getLines());
     String one = fileText.getFullText().toString().replace("\n", "\\n").replace("\r", "\\r");
     String two = anotherFileText.getFullText().toString().replace("\n", "\\n").replace("\r", "\\r");
     assertEquals(one, two);
 }

Result is

Expected :first line \r\n second line \n\r third line
Actual   :first line \n second line \n\n third line\n
  • Possible solution is to add new method or change existing FileSetCheck.process(File file, FileText fileText)
  • Add new method or change existing AbstractFileSetCheck.processFiltered(File file, List<String> lines)
  • And implement/change all occurrences of these methods in code base

What do you think? I can PR to show the problem

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 25, 2017

Member

AbstractFileSetCheck#processFiltered accepts List lines, after removing extension extends AbstractList it makes impossible to accept instances of FileText class.

change existing AbstractFileSetCheck.processFiltered(File file, List lines)

This is what I thought the point of the issue was. We are dropping 'extends AbstractList' from FileText and changing all methods who receive FileText indirectly by List<String> to now directly receive it as FileText.

Member

rnveach commented Jun 25, 2017

AbstractFileSetCheck#processFiltered accepts List lines, after removing extension extends AbstractList it makes impossible to accept instances of FileText class.

change existing AbstractFileSetCheck.processFiltered(File file, List lines)

This is what I thought the point of the issue was. We are dropping 'extends AbstractList' from FileText and changing all methods who receive FileText indirectly by List<String> to now directly receive it as FileText.

timurt added a commit to timurt/checkstyle that referenced this issue Jun 25, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jun 25, 2017

@MEZk MEZk moved this from To Do to In Progress in Flexible Suppression Model Jun 26, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jun 27, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jun 27, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 5, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 5, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 6, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 6, 2017

romani added a commit that referenced this issue Jul 6, 2017

@romani romani added this to the 8.1 milestone Jul 6, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 6, 2017

Member

fix is merged

Member

romani commented Jul 6, 2017

fix is merged

@romani romani closed this Jul 6, 2017

@MEZk MEZk moved this from In Progress to Done in Flexible Suppression Model Jul 6, 2017

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