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

remove FileContentsHolder module as FileContents object is available for filters on TreeWalker in TreeWalkerAudit Event #3573

Closed
romani opened this Issue Nov 23, 2016 · 15 comments

Comments

@romani
Member

romani commented Nov 23, 2016

This class - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/FileContentsHolder.java

The only reason of FileContentsHolder is to keep FileContents.
This is a HACK to keep file content as global variable to be accessible from all other places (class that wrap one field and provide simple getter for it).

Migration note for checkstyle users:
So FileContentsHolder is not required any more and just need to be removed from clients configurations.
In scope of #4714 , we moved FileContents object to TreeWalkerAuditEvent.

@romani romani added the checkstyle8 label Nov 23, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 23, 2016

Member

is HACK

My exact wording. :)
My proposal inside MultiThreaded Checkstyle thread was to merge this behavior into FileContents and pass that around to the Checks and Filters instead of FileText. We could expand on this and give FileContents special storage to allow Checks their own custom area for communication with custom filters.

Member

rnveach commented Nov 23, 2016

is HACK

My exact wording. :)
My proposal inside MultiThreaded Checkstyle thread was to merge this behavior into FileContents and pass that around to the Checks and Filters instead of FileText. We could expand on this and give FileContents special storage to allow Checks their own custom area for communication with custom filters.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 5, 2017

Contributor

@romani
Added to GSoC as it blocks #4382 and the solution based on the @rnveach's work will be a good starting point.

Contributor

MEZk commented Jun 5, 2017

@romani
Added to GSoC as it blocks #4382 and the solution based on the @rnveach's work will be a good starting point.

@MEZk MEZk added the GSoC2017 label Jun 5, 2017

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 14, 2017

Contributor

What should be done to resolve the issue:

Step 1

  1. Add the new class which will store FileText object and a set of localized messages. Proposed name is 'FileProcessingResult' (or @rnveach's class name).
  2. MessageDispatcher#fireErrors should receive an object of class FileProcessingResult. The signature of the public API is required to be changed:
void fireErrors(String fileName, FileProcessingResult fileProcessingResult);
  1. Fix AbstractFileSetCheck and Checker to work with the new method (fireErrors) signature.
  2. Change the signature of Checker#processFile. The method should return FileProcessingResult object.
  3. Fix Checker tests.
  4. Fix TranslationCheck.

Step 2

  1. Replace FileText with FileContents in FileProcessingResult class.
  2. Change signature of FileSetCheck#process in order to receive FileContents instead of List lines.
  3. Fix AbstractFileSetCheck.
  4. Fix all classes which implements FileSetCheck ('process' method signature).
  5. Fix failing tests.
  6. Add FileContents to audit event to make it available for filters.

Change the signature of AuditEvent constructor in order to receive FileProcessingResult:

public AuditEvent(Object src, String fileName, LocalizedMessage localizedMessage)

should become

public AuditEvent(Object src, FileProcessingResult fileProcessingResult)

We will add AST root to FileContents later in #4382

Step 3

  1. Remove FileContentsHolder (do not forget to remove it from configs, documentation, tests).
  2. Fix SuppressionCommentFilter. Remove 'fileContentsReference', 'getFileContents', 'setFileContents'. Change 'accept', 'tagSuppressions' methods.
  3. Fix SuppressWithNearbyCommentFilter.
  4. Fix failing tests.

Each step should be a separate PR.

@timurt
Please, start working on this task. You can use @rnveach's branch as a starting point.

Contributor

MEZk commented Jun 14, 2017

What should be done to resolve the issue:

Step 1

  1. Add the new class which will store FileText object and a set of localized messages. Proposed name is 'FileProcessingResult' (or @rnveach's class name).
  2. MessageDispatcher#fireErrors should receive an object of class FileProcessingResult. The signature of the public API is required to be changed:
void fireErrors(String fileName, FileProcessingResult fileProcessingResult);
  1. Fix AbstractFileSetCheck and Checker to work with the new method (fireErrors) signature.
  2. Change the signature of Checker#processFile. The method should return FileProcessingResult object.
  3. Fix Checker tests.
  4. Fix TranslationCheck.

Step 2

  1. Replace FileText with FileContents in FileProcessingResult class.
  2. Change signature of FileSetCheck#process in order to receive FileContents instead of List lines.
  3. Fix AbstractFileSetCheck.
  4. Fix all classes which implements FileSetCheck ('process' method signature).
  5. Fix failing tests.
  6. Add FileContents to audit event to make it available for filters.

Change the signature of AuditEvent constructor in order to receive FileProcessingResult:

public AuditEvent(Object src, String fileName, LocalizedMessage localizedMessage)

should become

public AuditEvent(Object src, FileProcessingResult fileProcessingResult)

We will add AST root to FileContents later in #4382

Step 3

  1. Remove FileContentsHolder (do not forget to remove it from configs, documentation, tests).
  2. Fix SuppressionCommentFilter. Remove 'fileContentsReference', 'getFileContents', 'setFileContents'. Change 'accept', 'tagSuppressions' methods.
  3. Fix SuppressWithNearbyCommentFilter.
  4. Fix failing tests.

Each step should be a separate PR.

@timurt
Please, start working on this task. You can use @rnveach's branch as a starting point.

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

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 14, 2017

Member

There is also alot of steps for just 1 issue, which makes think we should break this up somehow (multiple issues) if we do decide to do all this.
I'm not sure we can review all this in 1 PR too.

'FileProcessingResult'

I feel fixing current design of FileContentsHolder shouldn't include adding FileProcessingResult which is a big change by itself.
Please also allow @romani some time to review design decisions as we sometimes disagree. I'm not sure if he looked at my branch any as he never responded to google groups discussion.

Member

rnveach commented Jun 14, 2017

There is also alot of steps for just 1 issue, which makes think we should break this up somehow (multiple issues) if we do decide to do all this.
I'm not sure we can review all this in 1 PR too.

'FileProcessingResult'

I feel fixing current design of FileContentsHolder shouldn't include adding FileProcessingResult which is a big change by itself.
Please also allow @romani some time to review design decisions as we sometimes disagree. I'm not sure if he looked at my branch any as he never responded to google groups discussion.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 15, 2017

Contributor

@rnveach @timurt
Ok, lets wait for @romani .

Contributor

MEZk commented Jun 15, 2017

@rnveach @timurt
Ok, lets wait for @romani .

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 15, 2017

Member

@rnveach , @MEZk , @timurt , sorry for delay.

I did review of Steps 1-3, they looks ok, I do not have time to make deep analysis ...
I am ok to keep all in oner issue, but there should be at least 3 PRs (any number of PRs to ease code-review of changes.)

I will do 3.8.3 hot fix release till the end of week, and do upgrade to checkstyle 8.0 , so master branch will be ready for major changes in processing and APIs.

Member

romani commented Jun 15, 2017

@rnveach , @MEZk , @timurt , sorry for delay.

I did review of Steps 1-3, they looks ok, I do not have time to make deep analysis ...
I am ok to keep all in oner issue, but there should be at least 3 PRs (any number of PRs to ease code-review of changes.)

I will do 3.8.3 hot fix release till the end of week, and do upgrade to checkstyle 8.0 , so master branch will be ready for major changes in processing and APIs.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jun 19, 2017

Collaborator

Finished first step,
Had problem with Checker class,

Class Fan-Out Difficulty is 26

as I understood it is because of new class I ve created

Collaborator

timurt commented Jun 19, 2017

Finished first step,
Had problem with Checker class,

Class Fan-Out Difficulty is 26

as I understood it is because of new class I ve created

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 19, 2017

Contributor

Had problem with Checker class,

Answered in PR #4468.

Contributor

MEZk commented Jun 19, 2017

Had problem with Checker class,

Answered in PR #4468.

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

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

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

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

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

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

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

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

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

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

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

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

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

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 12, 2017

Member

Sorry for delay...

Change signature of FileSetCheck#process in order to receive FileContents instead of List lines.

here is conceptual problem. FileContents is TreeWalker's class, it contains comments (stuff from java AST) it should be used only in scope of TreeWalker.
FileSetCheck should stay on list of Lines and do processing as there is no java related Checks/modules at all around.

FileContetsHolder is used only in Filters. In current implementation all filters are in Checker (list of lines and AST modules owner), that is why such global object was used to deliver context of AST to Filters.

Checker should not know about AST at all, it operates by files and list of lines, that is all.
We need to move all AST related filters to TreeWalker, TreeWalker should become a parent for SuppressionCommentFilter and SuppressWithNearbyCommentFilter (the only filters that need FileContetsHolder).

There should be two types of filters:
Filter {boolean accept(AuditEvent event);} (at it is now , for any plain file processing as plain text)
TreeWalkerFilter {boolean accept(TreeWalkerAuditEvent event);} (NEW)

TreeWalkerAuditEvent { 
  String fileName, 
  FileContext filecontext;
  DetailAst root;
  AstLocalizedMessages messages;
}

All events that pass filters of TreeWalker are going to filters of parent (Checker), it is better to hide/remove any AST related context from Messages as Checker should not know/see them. We probably should make interface for LocalizedMessage and AstLocalizedMessage extends LocalizedMessage. So Checks creates AstLocalizedMessage but to Checker we give such instances by LocalizedMessage type, so Checker(with its filters) will not see AST fields.

@MEZk , please find me in skype to discuss this.

Member

romani commented Jul 12, 2017

Sorry for delay...

Change signature of FileSetCheck#process in order to receive FileContents instead of List lines.

here is conceptual problem. FileContents is TreeWalker's class, it contains comments (stuff from java AST) it should be used only in scope of TreeWalker.
FileSetCheck should stay on list of Lines and do processing as there is no java related Checks/modules at all around.

FileContetsHolder is used only in Filters. In current implementation all filters are in Checker (list of lines and AST modules owner), that is why such global object was used to deliver context of AST to Filters.

Checker should not know about AST at all, it operates by files and list of lines, that is all.
We need to move all AST related filters to TreeWalker, TreeWalker should become a parent for SuppressionCommentFilter and SuppressWithNearbyCommentFilter (the only filters that need FileContetsHolder).

There should be two types of filters:
Filter {boolean accept(AuditEvent event);} (at it is now , for any plain file processing as plain text)
TreeWalkerFilter {boolean accept(TreeWalkerAuditEvent event);} (NEW)

TreeWalkerAuditEvent { 
  String fileName, 
  FileContext filecontext;
  DetailAst root;
  AstLocalizedMessages messages;
}

All events that pass filters of TreeWalker are going to filters of parent (Checker), it is better to hide/remove any AST related context from Messages as Checker should not know/see them. We probably should make interface for LocalizedMessage and AstLocalizedMessage extends LocalizedMessage. So Checks creates AstLocalizedMessage but to Checker we give such instances by LocalizedMessage type, so Checker(with its filters) will not see AST fields.

@MEZk , please find me in skype to discuss this.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 12, 2017

Member

it is better to remove any AST related context from Messages as Checker should not know/see them

If each relies on specific interface, then IMO those fields will remain hidden. I don't feel we should completely remake the class just to hide fields otherwise.

Filter
TreeWalkerFilter

Should we name everything after main modules? CheckerFilter and TreeWalkerFilter. CheckerAuditEvent and TreeWalkerAuditEvent.
Should we assign some special interface to these modules that say they are their own central point for all children communication? Something similar to RootModule.

TreeWalkerAuditEvent

We should convert AuditEvent to an interface or abstract class and have CheckerAuditEvent and TreeWalkerAuditEvent implement/extend it.

Member

rnveach commented Jul 12, 2017

it is better to remove any AST related context from Messages as Checker should not know/see them

If each relies on specific interface, then IMO those fields will remain hidden. I don't feel we should completely remake the class just to hide fields otherwise.

Filter
TreeWalkerFilter

Should we name everything after main modules? CheckerFilter and TreeWalkerFilter. CheckerAuditEvent and TreeWalkerAuditEvent.
Should we assign some special interface to these modules that say they are their own central point for all children communication? Something similar to RootModule.

TreeWalkerAuditEvent

We should convert AuditEvent to an interface or abstract class and have CheckerAuditEvent and TreeWalkerAuditEvent implement/extend it.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 12, 2017

Member

Should we name everything after main modules? CheckerFilter and TreeWalkerFilter. CheckerAuditEvent and TreeWalkerAuditEvent

yes it is better to rename all now, it make code clear of context (what class belong to what context)(context is plain text or AST).

Should we assign some special interface to these modules that say they are their own central point for all children communication?

I am not sure what you mean by this. TreeWalkerFilter interface should be enough.

We should convert AuditEvent to an interface or abstract class and have CheckerAuditEvent and TreeWalkerAuditEvent implement/extend it.

ok, but lets hide implementations out of API package.

Member

romani commented Jul 12, 2017

Should we name everything after main modules? CheckerFilter and TreeWalkerFilter. CheckerAuditEvent and TreeWalkerAuditEvent

yes it is better to rename all now, it make code clear of context (what class belong to what context)(context is plain text or AST).

Should we assign some special interface to these modules that say they are their own central point for all children communication?

I am not sure what you mean by this. TreeWalkerFilter interface should be enough.

We should convert AuditEvent to an interface or abstract class and have CheckerAuditEvent and TreeWalkerAuditEvent implement/extend it.

ok, but lets hide implementations out of API package.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 12, 2017

Member

Should we assign some special interface to these modules

I am not sure what you mean by this.

I meant TreeWalker. Should we give it a special interface like CentralCommunicationModule or something saying it localizes all messages and filters inside it before passing them to parent. This would put them up on their own throne as they are not the same as checks or normal file sets anymore. This would probably more depend on if we need to recognize and process them separately from others.

Member

rnveach commented Jul 12, 2017

Should we assign some special interface to these modules

I am not sure what you mean by this.

I meant TreeWalker. Should we give it a special interface like CentralCommunicationModule or something saying it localizes all messages and filters inside it before passing them to parent. This would put them up on their own throne as they are not the same as checks or normal file sets anymore. This would probably more depend on if we need to recognize and process them separately from others.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 13, 2017

Contributor

@timurt
Steps 1, 2, and 3 are not required to do. Please see #4714 and @romani's comment for more details. In the scope of this issue we need to completely remove FileContentsHolder. This must be done only after #4714 when we remove its usage from the filters.

Contributor

MEZk commented Jul 13, 2017

@timurt
Steps 1, 2, and 3 are not required to do. Please see #4714 and @romani's comment for more details. In the scope of this issue we need to completely remove FileContentsHolder. This must be done only after #4714 when we remove its usage from the filters.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jul 29, 2017

Collaborator

@MEZk @romani

In the scope of this issue we need to completely remove FileContentsHolder

should I directly contribute to checktyle-tester or apply patch inside wercker.yml?

Collaborator

timurt commented Jul 29, 2017

@MEZk @romani

In the scope of this issue we need to completely remove FileContentsHolder

should I directly contribute to checktyle-tester or apply patch inside wercker.yml?

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

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

@romani romani changed the title from resovle design problem of FileContentsHolder to remove FileContentsHolder module as FileContents object is awailable for filters on TreeWalker in TreeWalkerAudit Event Jul 30, 2017

@romani romani changed the title from remove FileContentsHolder module as FileContents object is awailable for filters on TreeWalker in TreeWalkerAudit Event to remove FileContentsHolder module as FileContents object is available for filters on TreeWalker in TreeWalkerAudit Event Jul 30, 2017

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

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

romani added a commit that referenced this issue Aug 1, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 1, 2017

Member

fix is merged.

Member

romani commented Aug 1, 2017

fix is merged.

@romani romani closed this Aug 1, 2017

@romani romani added this to the 8.2 milestone Aug 1, 2017

@MEZk MEZk moved this from In Progress to Done in Flexible Suppression Model Aug 1, 2017

romani added a commit that referenced this issue Aug 3, 2017

romani added a commit that referenced this issue Aug 3, 2017

soon added a commit to soon/checkstyle that referenced this issue Aug 29, 2017

anton-johansson added a commit to viskan/checkstyle-configuration that referenced this issue Jan 9, 2018

Fix breaking changes for Checkstyle 8.7
Move `SuppressionCommentFilter` to `TreeWalker` (checkstyle/checkstyle#4714).
Remove `FileContentHolder` as it is no longer needed (checkstyle/checkstyle#3573).
Remove `maxLineLength` of `LeftCurlyCheck` as it has been removed (checkstyle/checkstyle#3671).

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