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

Service Provider for applying enhancements #1369

Merged
merged 15 commits into from
Jul 18, 2024

Conversation

matakleo
Copy link
Collaborator

@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
Copy link
Member

@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!

cdm/core/src/main/java/ucar/nc2/filter/Classifier.java Outdated Show resolved Hide resolved
cdm/core/src/main/java/ucar/nc2/dataset/VariableDS.java Outdated Show resolved Hide resolved
cdm/core/src/main/java/ucar/nc2/dataset/VariableDS.java Outdated Show resolved Hide resolved
cdm/core/src/main/java/ucar/nc2/filter/ScaleOffset.java Outdated Show resolved Hide resolved
cdm/core/src/main/java/ucar/nc2/filter/Classifier.java Outdated Show resolved Hide resolved

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

Enhancement Create(VariableDS var);
Copy link
Member

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

Choose a reason for hiding this comment

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

+1

Copy link

@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);

Choose a reason for hiding this comment

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

+1

@@ -116,4 +125,21 @@ public static int[] stringToIntArray(String str) {
return intArray;
}

public static class Provider implements EnhancementProvider {

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);
}

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
8 checks passed
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