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

Normalize fields fix #386

Merged
merged 3 commits into from
Jun 1, 2021
Merged

Normalize fields fix #386

merged 3 commits into from
Jun 1, 2021

Conversation

binh-dam-ibigroup
Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup commented May 28, 2021

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
  • [na] The description lists all applicable issues this PR seeks to resolve
  • [na] The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

This PR supplements #371 by making NormalizeFieldTransformation.substitutions and NormalizeFieldTransformation.capitalizationExceptions not null if no default substitutions are specified in the configurations (at least an empty list is returned).

Copy link
Contributor

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Looks pretty good, only comment is that the test doesn't really verify that it successfully returns a list with two objects for the JSON_LIST arg. I don't really find the parameterized test cases that useful here -- it might be more effective to have two tests: one that checks null input is an empty list, and one that checks that it parses JSON_LIST as Substitutions.

@binh-dam-ibigroup
Copy link
Contributor Author

Looks pretty good, only comment is that the test doesn't really verify that it successfully returns a list with two objects for the JSON_LIST arg. I don't really find the parameterized test cases that useful here -- it might be more effective to have two tests: one that checks null input is an empty list, and one that checks that it parses JSON_LIST as Substitutions.

Right. Updated in 06d46fe.

Copy link
Contributor

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Perfect!

Copy link
Contributor

@br648 br648 left a comment

Choose a reason for hiding this comment

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

LGTM

@br648 br648 removed their assignment Jun 1, 2021
@landonreed landonreed merged commit 69d7c9c into dev Jun 1, 2021
@landonreed landonreed deleted the normalize-fields-fix branch June 1, 2021 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants