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.

Sign up for GitHub

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 -Xlint exclusions in the ingest-common module. #40505

Merged
merged 2 commits into from
Mar 29, 2019
Merged

Remove -Xlint exclusions in the ingest-common module. #40505

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8d9ab6
Remove -Xlint exclusions in the ingest-common module.
martijnvg Mar 27, 2019
c3746d8
Keep the generics
martijnvg Mar 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions modules/ingest-common/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,6 @@ dependencies {
compile project(':libs:dissect')
}

compileJava.options.compilerArgs << "-Xlint:-unchecked,-rawtypes"
compileTestJava.options.compilerArgs << "-Xlint:-unchecked,-rawtypes"

integTestCluster {
module project(':modules:lang-painless')
}
6 changes: 2 additions & 4 deletions .../ingest-common/src/main/java/org/elasticsearch/ingest/common/AbstractStringProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,8 @@
/**
* Base class for processors that manipulate source strings and require a single "fields" array config value, which
* holds a list of field names in string format.
*
* @param <T> The resultant type for the target field
*/
abstract class AbstractStringProcessor<T> extends AbstractProcessor {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's more classes to change, but I'm wondering if ti's worth changing the classes that extend from this to include a type instead like so:

public final class BytesProcessor extends AbstractStringProcessor<Long> {

This would allow us to keep type safety for the process issue

Copy link
Member Author

Choose a reason for hiding this comment

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

I can go either way. The type wasn't really used, so I thought I should remove it, but what you;re suggestion is good too, as it remove those -Xlint exclusions too. @jakelandis what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to leave the generic type there. I think I added it, but didn't bother with explicitly satisfying the generic since is implicitly satisfied by the abstract T process method.

It looks like the lint exclusions pre-dated this change, and not sure if explicitly satisfying the generic will allow for a clean removal of the exclusion. (I am bit surprised that implicitly satisfying generics trigger lint warnings)

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed: c3746d8

abstract class AbstractStringProcessor extends AbstractProcessor {
private final String field;
private final boolean ignoreMissing;
private final String targetField;
Expand Down Expand Up @@ -70,7 +68,7 @@ public final IngestDocument execute(IngestDocument document) {
return document;
}

protected abstract T process(String value);
protected abstract Object process(String value);

abstract static class Factory implements Processor.Factory {
final String processorType;
Expand Down
8 changes: 4 additions & 4 deletions ...common/src/test/java/org/elasticsearch/ingest/common/AbstractStringProcessorTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,18 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;

public abstract class AbstractStringProcessorTestCase<T> extends ESTestCase {
public abstract class AbstractStringProcessorTestCase extends ESTestCase {

protected abstract AbstractStringProcessor newProcessor(String field, boolean ignoreMissing, String targetField);

protected String modifyInput(String input) {
return input;
}

protected abstract T expectedResult(String input);
protected abstract Object expectedResult(String input);

protected Class<T> expectedResultType(){
return (Class<T>) String.class; // most results types are Strings
protected Class<?> expectedResultType(){
return String.class; // most results types are Strings
}

public void testProcessor() throws Exception {
Expand Down