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

Updated model and removed author service #35

Merged
merged 9 commits into from
Oct 20, 2017

Conversation

yolandeleungFT
Copy link
Contributor

No description provided.

Copy link
Contributor

@izzyblues izzyblues left a comment

Choose a reason for hiding this comment

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

Except the two minor comments, the rest looks good to me.

default:
return esModel, nil
}
}

func (mp *EsModelPopulator) ConvertAggregateConceptToESConceptModel(concept AggregateConceptModel, conceptType string, publishRef string) (interface{}, error) {
func ConvertAggregateConceptToESConceptModel(concept AggregateConceptModel, conceptType string, publishRef string) (interface{}, error) {
esModel := convertAggregateConceptToESConceptModel(concept, conceptType, publishRef)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment to above... I think that you can get rid of this function just by using the code at line 55.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

func (mp *EsModelPopulator) ConvertConceptToESConceptModel(concept ConceptModel, conceptType string, publishRef string) (interface{}, error) {
func ConvertConceptToESConceptModel(concept ConceptModel, conceptType string, publishRef string) (interface{}, error) {
esModel := convertToESConceptModel(concept, conceptType, publishRef)
Copy link
Contributor

Choose a reason for hiding this comment

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

Something really minor. I think that you can get rid of this function just by using the code at line 59. In my opinion, convertToESConceptModel has confusing name and it is useless since it is implemented by a single line of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 81.433% when pulling 0883567 on remove_author_lookup into d49b20e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 81.433% when pulling 2931b9a on remove_author_lookup into d49b20e on master.

izzyblues
izzyblues previously approved these changes Oct 18, 2017
galiri
galiri previously approved these changes Oct 18, 2017
@yolandeleungFT yolandeleungFT dismissed stale reviews from galiri and izzyblues via 43be653 October 18, 2017 11:44
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 81.433% when pulling a3999b3 on remove_author_lookup into d49b20e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 81.433% when pulling 2c6126a on remove_author_lookup into d49b20e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 81.433% when pulling 7ac023b on remove_author_lookup into d49b20e on master.

izzyblues
izzyblues previously approved these changes Oct 18, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 81.107% when pulling 3d238a4 on remove_author_lookup into d49b20e on master.

if err != nil {
return nil, err
isFTAuthor := strconv.FormatBool(concept.IsAuthor)
if isFTAuthor == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since strconv.FormatBool returns only "true" or "false" strings, this condition is never true. So, this if statement is not necessary anymore.

}
return *person, nil
return *esPersonModel, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above about pointers

}
return *person, nil
return *esPersonModel, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confused... esPersonModel is returned as a pointer to a pointer of EsPersonConceptModel (because you used & at line 25 and * at line 29) and esModel is returned as copy... I have the impression that you can simply return both variables as a copy.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.5%) to 81.311% when pulling e24b217 on remove_author_lookup into d49b20e on master.

@yolandeleungFT yolandeleungFT merged commit 021eee9 into master Oct 20, 2017
@erdema-ft erdema-ft deleted the remove_author_lookup branch August 20, 2018 13:52
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.

None yet

4 participants