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

Issue #3573: Resolve design problem of FileContentsHolder (step 1) #4468

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions config/intellij-idea-inspections.xml
Expand Up @@ -317,6 +317,10 @@
</inspection_tool>
<inspection_tool class="ClassWithOnlyPrivateConstructors" enabled="true" level="WARNING" enabled_by_default="true" />
<inspection_tool class="ClassWithTooManyDependencies" enabled="true" level="ERROR" enabled_by_default="true">
<scope name="Tests" level="ERROR" enabled="true">
<!-- we have to use it because number of dependencies of CheckerTest exceeds 33. -->
<option name="limit" value="40" />
</scope>
Copy link
Member

Choose a reason for hiding this comment

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

if there is problem in one file for whole rule, we do not change a rule, we do suppression in certain class.
please use javadoc tag suppression for CheckerTest.

<option name="limit" value="33" />
</inspection_tool>
<inspection_tool class="ClassWithTooManyDependents" enabled="true" level="ERROR" enabled_by_default="true">
Expand Down Expand Up @@ -2014,6 +2018,7 @@
<option value="ProhibitedExceptionThrown" />
<!-- we have to use it when pass null argument in test purporses -->
<option value="NullArgumentToVariableArgMethod" />
<option value="ClassWithTooManyDependencies" />
</list>
</option>
</inspection_tool>
Expand Down
30 changes: 18 additions & 12 deletions src/main/java/com/puppycrawl/tools/checkstyle/Checker.java
Expand Up @@ -45,6 +45,7 @@
import com.puppycrawl.tools.checkstyle.api.Configuration;
import com.puppycrawl.tools.checkstyle.api.Context;
import com.puppycrawl.tools.checkstyle.api.ExternalResourceHolder;
import com.puppycrawl.tools.checkstyle.api.FileProcessingResult;
import com.puppycrawl.tools.checkstyle.api.FileSetCheck;
import com.puppycrawl.tools.checkstyle.api.FileText;
import com.puppycrawl.tools.checkstyle.api.Filter;
Expand All @@ -63,6 +64,8 @@
* @author lkuehne
* @author Andrei Selkin
*/
// -@cs[ClassFanOutComplexity] Added to suppress error, now number of classes exceeds 25.
// See https://github.com/checkstyle/checkstyle/issues/4478
public class Checker extends AutomaticBean implements MessageDispatcher, RootModule {
/** Message to use when an exception occurs and should be printed as a violation. */
public static final String EXCEPTION_MSG = "general.exception";
Expand Down Expand Up @@ -283,8 +286,8 @@ private void processFiles(List<File> files) throws CheckstyleException {
cache.put(fileName, timestamp);
}
fireFileStarted(fileName);
final SortedSet<LocalizedMessage> fileMessages = processFile(file);
fireErrors(fileName, fileMessages);
final FileProcessingResult fileProcessingResult = processFile(file);
fireErrors(fileProcessingResult);
fireFileFinished(fileName);
}
// -@cs[IllegalCatch] There is no other way to deliver filename that was under
Expand All @@ -304,16 +307,18 @@ private void processFiles(List<File> files) throws CheckstyleException {
/**
* Processes a file with all FileSetChecks.
* @param file a file to process.
* @return a sorted set of messages to be logged.
* @return file processing results object.
* @throws CheckstyleException if error condition within Checkstyle occurs.
* @noinspection ProhibitedExceptionThrown
*/
private SortedSet<LocalizedMessage> processFile(File file) throws CheckstyleException {
private FileProcessingResult processFile(File file) throws CheckstyleException {
Copy link
Contributor

Choose a reason for hiding this comment

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

method javadoc should be corrected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

final SortedSet<LocalizedMessage> fileMessages = new TreeSet<>();
FileText fileText = null;

try {
final FileText theText = new FileText(file.getAbsoluteFile(), charset);
fileText = new FileText(file.getAbsoluteFile(), charset);
for (final FileSetCheck fsc : fileSetChecks) {
fileMessages.addAll(fsc.process(file, theText));
fileMessages.addAll(fsc.process(file, fileText));
}
}
catch (final IOException ioe) {
Expand All @@ -340,7 +345,7 @@ private SortedSet<LocalizedMessage> processFile(File file) throws CheckstyleExce
new String[] {sw.getBuffer().toString()},
null, getClass(), null));
}
return fileMessages;
return new FileProcessingResult(file.getAbsolutePath(), fileText, fileMessages);
}

/**
Expand Down Expand Up @@ -373,15 +378,16 @@ public void fireFileStarted(String fileName) {
/**
* Notify all listeners about the errors in a file.
*
* @param fileName the audited file
* @param errors the audit errors from the file
* @param fileProcessingResult file processing results object
*/
@Override
public void fireErrors(String fileName, SortedSet<LocalizedMessage> errors) {
public void fireErrors(FileProcessingResult fileProcessingResult) {
Copy link
Member

Choose a reason for hiding this comment

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

fileProcessingResult.getFileText is not used it is already sign smth is wrong in design.

please see my comment at #3573 (comment)

final String fileName = fileProcessingResult.getFilePath();
final String stripped = CommonUtils.relativizeAndNormalizePath(basedir, fileName);
boolean hasNonFilteredViolations = false;
for (final LocalizedMessage element : errors) {
final AuditEvent event = new AuditEvent(this, stripped, element);
final SortedSet<LocalizedMessage> errorMessages = fileProcessingResult.getMessages();
for (LocalizedMessage errorMessage : errorMessages) {
final AuditEvent event = new AuditEvent(this, stripped, errorMessage);
Copy link
Member

Choose a reason for hiding this comment

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

Step 2
6. Add FileContents to audit event to make it available for filters. We will add AST root to FileContents later

@MEZk Shouldn't the event eventually accept FileProcessingResult?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rnveach
Maybe, but this should not be done in this PR. I'll think about it.

Copy link
Contributor

@MEZk MEZk Jun 22, 2017

Choose a reason for hiding this comment

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

@rnveach
You are right. AuditEvent should receive FileProcessingResult. Changed the plan.
#3573 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

ok.

if (filters.accept(event)) {
hasNonFilteredViolations = true;
for (final AuditListener listener : listeners) {
Expand Down
Expand Up @@ -170,6 +170,6 @@ public final void log(int lineNo, int colNo, String key,
protected final void fireErrors(String fileName) {
final SortedSet<LocalizedMessage> errors = new TreeSet<>(messageCollector);
messageCollector.clear();
getMessageDispatcher().fireErrors(fileName, errors);
getMessageDispatcher().fireErrors(new FileProcessingResult(fileName, null, errors));
}
}
@@ -0,0 +1,85 @@
////////////////////////////////////////////////////////////////////////////////
// checkstyle: Checks Java source code for adherence to a set of rules.
// Copyright (C) 2001-2017 the original author or authors.
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; either
// version 2.1 of the License, or (at your option) any later version.
//
// This library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
// Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public
// License along with this library; if not, write to the Free Software
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
////////////////////////////////////////////////////////////////////////////////

package com.puppycrawl.tools.checkstyle.api;

import java.util.Collections;
import java.util.SortedSet;
import java.util.TreeSet;

/**
* Represents the file processing results.
*
* @author Timur Tibeyev
*/
public class FileProcessingResult {
/** The contents of the file. */
private final FileText fileText;

/** The sorted set of messages. */
private final SortedSet<LocalizedMessage> messages;

/** The file path. */
private final String filePath;

/**
* Creates a new {@code FileProcessingResult} instance.
*
* @param filePath absolute path to the file
* @param fileText the contents of the file
* @param messages the sorted set of messages
*/
public FileProcessingResult(String filePath, FileText fileText,
SortedSet<LocalizedMessage> messages) {
this.filePath = filePath;
if (fileText == null) {
this.fileText = null;
}
else {
this.fileText = new FileText(fileText);
}
final SortedSet<LocalizedMessage> validCopy = new TreeSet<>(messages);
this.messages = Collections.unmodifiableSortedSet(validCopy);
}

/**
* Returns the contents of the file.
* @return an object containing the full text of the file
*/
public FileText getFileText() {
return fileText;
}

/**
* Returns the sorted set of messages.
* @return the sorted set of messages
*/
public SortedSet<LocalizedMessage> getMessages() {
final SortedSet<LocalizedMessage> validCopy = new TreeSet<>(messages);
return Collections.unmodifiableSortedSet(validCopy);
}

/**
* Returns the file path.
* @return the file path
*/
public String getFilePath() {
return filePath;
}
}
Expand Up @@ -19,8 +19,6 @@

package com.puppycrawl.tools.checkstyle.api;

import java.util.SortedSet;

/**
* Used by FileSetChecks to distribute AuditEvents to AuditListeners.
* @author lkuehne
Expand All @@ -40,8 +38,7 @@ public interface MessageDispatcher {

/**
* Notify all listeners about the errors in a file.
* @param fileName the audited file
* @param errors the audit errors from the file
* @param fileProcessingResult file processing results object
*/
void fireErrors(String fileName, SortedSet<LocalizedMessage> errors);
void fireErrors(FileProcessingResult fileProcessingResult);
}
Expand Up @@ -45,6 +45,7 @@
import com.google.common.io.Closeables;
import com.puppycrawl.tools.checkstyle.Definitions;
import com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck;
import com.puppycrawl.tools.checkstyle.api.FileProcessingResult;
import com.puppycrawl.tools.checkstyle.api.FileText;
import com.puppycrawl.tools.checkstyle.api.LocalizedMessage;
import com.puppycrawl.tools.checkstyle.api.MessageDispatcher;
Expand Down Expand Up @@ -520,7 +521,7 @@ private void logIoException(IOException exception, File file) {
getClass(), null);
final SortedSet<LocalizedMessage> messages = new TreeSet<>();
messages.add(message);
getMessageDispatcher().fireErrors(file.getPath(), messages);
getMessageDispatcher().fireErrors(new FileProcessingResult(file.getPath(), null, messages));
LOG.debug("IOException occurred.", exception);
}

Expand Down
12 changes: 7 additions & 5 deletions src/test/java/com/puppycrawl/tools/checkstyle/CheckerTest.java
Expand Up @@ -65,6 +65,7 @@
import com.puppycrawl.tools.checkstyle.api.Context;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.ExternalResourceHolder;
import com.puppycrawl.tools.checkstyle.api.FileProcessingResult;
import com.puppycrawl.tools.checkstyle.api.FileText;
import com.puppycrawl.tools.checkstyle.api.Filter;
import com.puppycrawl.tools.checkstyle.api.FilterSet;
Expand All @@ -76,6 +77,7 @@
import com.puppycrawl.tools.checkstyle.filters.SuppressionFilter;
import com.puppycrawl.tools.checkstyle.utils.CommonUtils;

@SuppressWarnings("ClassWithTooManyDependencies")
public class CheckerTest extends BaseCheckTestSupport {

@Rule
Expand Down Expand Up @@ -112,7 +114,7 @@ public void testDestroy() throws Exception {
final SortedSet<LocalizedMessage> messages = new TreeSet<>();
messages.add(new LocalizedMessage(0, 0, "a Bundle", "message.key",
new Object[] {"arg"}, null, getClass(), null));
checker.fireErrors("Some File Name", messages);
checker.fireErrors(new FileProcessingResult("Some File Name", null, messages));

assertFalse("Checker.destroy() doesn't remove listeners.", auditAdapter.wasCalled());
assertFalse("Checker.destroy() doesn't remove filters.", filter.wasCalled());
Expand Down Expand Up @@ -145,7 +147,7 @@ public void testAddListener() throws Exception {
final SortedSet<LocalizedMessage> messages = new TreeSet<>();
messages.add(new LocalizedMessage(0, 0, "a Bundle", "message.key",
new Object[] {"arg"}, null, getClass(), null));
checker.fireErrors("Some File Name", messages);
checker.fireErrors(new FileProcessingResult("Some File Name", null, messages));
assertTrue("Checker.fireErrors() doesn't call listener", auditAdapter.wasCalled());
}

Expand Down Expand Up @@ -186,7 +188,7 @@ public void testRemoveListener() throws Exception {
final SortedSet<LocalizedMessage> messages = new TreeSet<>();
messages.add(new LocalizedMessage(0, 0, "a Bundle", "message.key",
new Object[] {"arg"}, null, getClass(), null));
checker.fireErrors("Some File Name", messages);
checker.fireErrors(new FileProcessingResult("Some File Name", null, messages));
assertTrue("Checker.fireErrors() doesn't call listener", aa2.wasCalled());
assertFalse("Checker.fireErrors() does call removed listener", auditAdapter.wasCalled());

Expand Down Expand Up @@ -230,7 +232,7 @@ public void testAddFilter() {
final SortedSet<LocalizedMessage> messages = new TreeSet<>();
messages.add(new LocalizedMessage(0, 0, "a Bundle", "message.key",
new Object[] {"arg"}, null, getClass(), null));
checker.fireErrors("Some File Name", messages);
checker.fireErrors(new FileProcessingResult("Some File Name", null, messages));
assertTrue("Checker.fireErrors() doesn't call filter", filter.wasCalled());
}

Expand All @@ -247,7 +249,7 @@ public void testRemoveFilter() {
final SortedSet<LocalizedMessage> messages = new TreeSet<>();
messages.add(new LocalizedMessage(0, 0, "a Bundle", "message.key",
new Object[] {"arg"}, null, getClass(), null));
checker.fireErrors("Some File Name", messages);
checker.fireErrors(new FileProcessingResult("Some File Name", null, messages));
assertTrue("Checker.fireErrors() doesn't call filter", f2.wasCalled());
assertFalse("Checker.fireErrors() does call removed filter", filter.wasCalled());

Expand Down
@@ -0,0 +1,62 @@
////////////////////////////////////////////////////////////////////////////////
// checkstyle: Checks Java source code for adherence to a set of rules.
// Copyright (C) 2001-2017 the original author or authors.
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; either
// version 2.1 of the License, or (at your option) any later version.
//
// This library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
// Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public
// License along with this library; if not, write to the Free Software
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
////////////////////////////////////////////////////////////////////////////////

package com.puppycrawl.tools.checkstyle.api;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;

import java.io.File;
import java.io.IOException;
import java.util.SortedSet;
import java.util.TreeSet;

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

public class FileProcessingResultTest {
@Rule
public final TemporaryFolder temporaryFolder = new TemporaryFolder();

@Test
public void testNonNullFileText() throws IOException {
final String charsetName = "ISO-8859-1";
final File file = temporaryFolder.newFile("test.java");
final FileText fileText = new FileText(file, charsetName);
final SortedSet<LocalizedMessage> messages = new TreeSet<>();
messages.add(new LocalizedMessage(0, 0, "a Bundle", "message.key",
new Object[] {"arg"}, null, getClass(), null));
final FileProcessingResult processingResult = new FileProcessingResult(
file.getAbsolutePath(), fileText, messages);
assertNotNull(processingResult.getFileText());
}

@Test
public void testNullFileText() {
final SortedSet<LocalizedMessage> messages = new TreeSet<>();
messages.add(new LocalizedMessage(0, 0, "a Bundle", "message.key",
new Object[] {"arg"}, null, getClass(), null));
final FileProcessingResult processingResult = new FileProcessingResult("Some file name",
null, messages);
assertEquals(1, processingResult.getMessages().size());
assertNull(processingResult.getFileText());
}
}