Skip to content

Conversation

@mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented Aug 24, 2020

Summary

  • Add datafile accessor methods/functionalities

Testing

@mnoman09 mnoman09 requested a review from a team as a code owner August 24, 2020 14:44
@mnoman09 mnoman09 removed their assignment Aug 24, 2020
@coveralls
Copy link

coveralls commented Aug 24, 2020

Pull Request Test Coverage Report for Build 1500

  • 11 of 11 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 89.497%

Totals Coverage Status
Change from base Build 1489: 0.1%
Covered Lines: 4090
Relevant Lines: 4570

💛 - Coveralls
@msohailhussain
Copy link
Contributor

unit test missing. toDatafile

public String toString() {
return "ProjectConfig{" +
"accountId='" + accountId + '\'' +
", datafile='" + datafile + '\'' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't add it in toString.
@mikeng13 @mjc1283 what do you suggest?

removed datafile from to string
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

A change requested

@mnoman09 mnoman09 removed their assignment Aug 26, 2020
private Map<String, OptimizelyFeature> featuresMap;
private String revision;

@JsonIgnore
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to remove @JsonIgnore redundant below at "getDatafile()".
Is it ok to remove @JsonIgnore for datafile completely?

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm

@jaeopt jaeopt merged commit a256a2e into master Aug 27, 2020
@jaeopt jaeopt deleted the mnoman/DatafileAccessor branch August 27, 2020 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants