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

proto elastic suggester #1549

Merged
merged 31 commits into from
Nov 12, 2019
Merged

proto elastic suggester #1549

merged 31 commits into from
Nov 12, 2019

Conversation

lionelB
Copy link
Contributor

@lionelB lionelB commented Oct 23, 2019

wip

@rmelisson
Copy link
Contributor

rmelisson commented Nov 4, 2019

Fix #1540 #1542 #841

@rmelisson rmelisson marked this pull request as ready for review November 5, 2019 11:24
@rmelisson
Copy link
Contributor

whoops, my bad c'est toujours WIP, je crois pas que je puisse revert de open à draft

@rmelisson rmelisson force-pushed the suggester-elastic-search branch from bd1da7e to 1984f54 Compare November 6, 2019 10:42
}

test("return suggestions for re in the right format", () =>
getSuggestions("re")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you use async in test case ? It would be easier to read.

test("return suggestions for re in the right format", async () => {
 const response = await request(app.callback()).get(`/api/v1/suggest?q=re`);
 expect(response.status).toBe(200)
 expect(response.get("Content-type").toMatch(/json/);
 expect(response.body).toMatchSnapshot()
})

if can also create a getSuggestion helder

async function getSuggestion(query) {
   return request(app.callback()).get(`/api/v1/suggest?q=${query}`);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

getSuggestions still return a promise, but the test cases are now using async for readability. Hope you're fine with this.

@lionelB
Copy link
Contributor Author

lionelB commented Nov 8, 2019

@rmelisson Looks good. just few comments :) nice job!

@SocialGroovyBot SocialGroovyBot requested a deployment to development November 12, 2019 10:18 Abandoned
@rmelisson rmelisson merged commit 080744b into master Nov 12, 2019
@rmelisson rmelisson deleted the suggester-elastic-search branch November 12, 2019 10:25
@revolunet
Copy link
Member

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.

5 participants