Skip to content

Service Provider for applying enhancements#1369

Merged
haileyajohnson merged 15 commits into
Unidata:maint-5.xfrom
matakleo:ClassifierServiceProvider
Jul 18, 2024
Merged

Service Provider for applying enhancements#1369
haileyajohnson merged 15 commits into
Unidata:maint-5.xfrom
matakleo:ClassifierServiceProvider

Conversation

@matakleo
Copy link
Copy Markdown
Contributor

@matakleo matakleo commented Jul 16, 2024

Description of Changes

PR Checklist

  • Link to any issues that the PR addresses
  • Add labels
  • Open as a draft PR
    until ready for review
  • Make sure GitHub tests pass
  • Mark PR as "Ready for Review"

@matakleo matakleo added the enhancement New feature or request label Jul 16, 2024
@matakleo matakleo marked this pull request as ready for review July 16, 2024 17:23
@matakleo matakleo requested a review from tdrwenski as a code owner July 16, 2024 17:24
@tdrwenski tdrwenski requested a review from haileyajohnson July 16, 2024 18:26
Copy link
Copy Markdown
Contributor

@tdrwenski tdrwenski left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a few minor style issues!

Comment thread cdm/core/src/main/java/ucar/unidata/io/spi/EnhancementProvider.java Outdated
Comment thread cdm/core/src/main/java/ucar/nc2/filter/Classifier.java Outdated
Comment thread cdm/core/src/main/java/ucar/nc2/dataset/VariableDS.java Outdated
Comment thread cdm/core/src/main/java/ucar/nc2/dataset/VariableDS.java Outdated
Comment thread cdm/core/src/main/java/ucar/nc2/filter/ScaleOffset.java Outdated
Comment thread cdm/core/src/main/java/ucar/nc2/filter/EnhancementProvider.java Outdated
Comment thread cdm/core/src/main/java/ucar/nc2/filter/Classifier.java Outdated
Comment thread cdm/core/src/main/java/ucar/nc2/filter/EnhancementProvider.java Outdated
Comment thread cdm/core/src/main/java/ucar/nc2/filter/EnhancementProvider.java Outdated
Comment thread cdm/core/src/main/java/ucar/nc2/filter/EnhancementProvider.java Outdated

boolean appliesTo(Enhance enhance, AttributeContainer attributes, DataType dt);

Enhancement Create(VariableDS var);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should start with lower case for method names in java, so let's make it create

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@haileyajohnson haileyajohnson force-pushed the ClassifierServiceProvider branch from 7dd1467 to 7ee573e Compare July 17, 2024 21:23
Copy link
Copy Markdown
Contributor

@haileyajohnson haileyajohnson left a comment

Choose a reason for hiding this comment

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

This looks great!


boolean appliesTo(Enhance enhance, AttributeContainer attributes, DataType dt);

Enhancement Create(VariableDS var);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

}

public static class Provider implements EnhancementProvider {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if you make another commit, remove these spaces just to keep things clean

@@ -290,5 +290,8 @@ public int getId() {
public Filter create(Map<String, Object> properties) {
return new ScaleOffset(properties);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no need to commit changes to this file at all it seems :)

@haileyajohnson haileyajohnson merged commit 57cb291 into Unidata:maint-5.x Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants