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

Feature/uppsf 836 change replace endpoint #43

Merged
merged 3 commits into from
Oct 31, 2019

Conversation

ivan-p-nikolov
Copy link
Contributor

Enable replace annotation endpoint to accept both concept id and predicate for substitution.
Add http://www.ft.com/ontology/hasBrand as a valid predicate identifier.
Update healthcheck panic guide urls.
Add documentation for replace endpoint.

@ivan-p-nikolov ivan-p-nikolov requested a review from a team October 29, 2019 11:17
@mchompalova mchompalova self-requested a review October 29, 2019 14:13
newHash := randomdata.RandStringRunes(56)

const contentID = "83a201c6-60cd-11e7-91a7-502f7ee26895"
fromAnnotationAPI := []annotations.Annotation{
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use expectedAnnotations.Annotations for input and move the response slice outside the test to be consistent with the other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I explicitly did not reuse any of the already defined structures. I feel that keeping the data next to the code makes for more readable tests and less fragile test suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree but if we don't reuse the structures, our code will become very large.

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid the code getting too large with such structures we can use fixtures.

mchompalova
mchompalova previously approved these changes Oct 30, 2019
Copy link
Contributor

@MiroslavGatsanoga MiroslavGatsanoga left a comment

Choose a reason for hiding this comment

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

There is a ticket (UPPSF-765) regarding fixing the docs for this service, could you update the README according to that?

newHash := randomdata.RandStringRunes(56)

const contentID = "83a201c6-60cd-11e7-91a7-502f7ee26895"
fromAnnotationAPI := []annotations.Annotation{
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid the code getting too large with such structures we can use fixtures.

handler/handler_test.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 31, 2019

Coverage Status

Coverage remained the same at 92.281% when pulling fa39a18 on feature/UPPSF-836-change-replace-endpoint into 4126bc7 on master.

mchompalova
mchompalova previously approved these changes Oct 31, 2019
Copy link
Contributor

@MiroslavGatsanoga MiroslavGatsanoga left a comment

Choose a reason for hiding this comment

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

Seems ok, you can squash the last two commits in some of the previous ones, so that we get cleaner history when we merge to master.

@ivan-p-nikolov ivan-p-nikolov merged commit bbc057f into master Oct 31, 2019
@ivan-p-nikolov ivan-p-nikolov deleted the feature/UPPSF-836-change-replace-endpoint branch October 31, 2019 11:27
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