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

Move common scope from search api to a new file #3108

Merged
merged 1 commit into from Jul 23, 2018

Conversation

Projects
None yet
4 participants
@stefannibrasil
Collaborator

stefannibrasil commented Jul 20, 2018

Fixes #3070.

(we commented the unit test for this for now because we are going to separate a time only for testing! but by running the api docs locally we could still get the same results as before, so apparently it didn't changed the fetched results)

@wafflebot wafflebot bot added the in progress label Jul 20, 2018

@stefannibrasil stefannibrasil referenced this pull request Jul 20, 2018

Closed

Refactor Search API #3070

3 of 3 tasks complete
@plotsbot

This comment has been minimized.

Collaborator

plotsbot commented Jul 20, 2018

1 Message
📖 @stefannibrasil Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

@wafflebot wafflebot bot removed the in progress label Jul 20, 2018

@wafflebot wafflebot bot added the in progress label Jul 20, 2018

@stefannibrasil stefannibrasil force-pushed the milaaraujo:srch-api-scope branch from d0435ab to 25f1abe Jul 20, 2018

@stefannibrasil stefannibrasil force-pushed the milaaraujo:srch-api-scope branch from 25f1abe to bc4d6e5 Jul 20, 2018

@milaaraujo milaaraujo self-assigned this Jul 20, 2018

@@ -125,7 +79,8 @@ def search_profiles(search_string, limit = 5)
def search_notes(search_string, limit = 5)

This comment has been minimized.

@jywarren

jywarren Jul 20, 2018

Contributor

Maybe we should add to the comment to say "and package up as a TagResult" for clarity?

This comment has been minimized.

@stefannibrasil

stefannibrasil Jul 21, 2018

Collaborator

yeah, good point!

@jywarren

This comment has been minimized.

Contributor

jywarren commented Jul 20, 2018

This is looking good - so when you add the test back in, and are ready for final review, just ping me!

These files are getting much simpler, very nice. Keep an eye out for layers of abstraction that seem unnecessary! And keep in mind how we want to be able to tell new people about how this is organized - it should be easy for a newcomer to figure out how it's all wired together, and to make a change or addition.

Thanks, great work!

@stefannibrasil stefannibrasil force-pushed the milaaraujo:srch-api-scope branch from bc4d6e5 to 707fa83 Jul 21, 2018

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Jul 21, 2018

you're right, @jywarren I added some notes that I think make the changes more clear now, let me know what do you think :) I also fixed the test that was commented, thanks!

@jywarren jywarren merged commit 5574837 into publiclab:master Jul 23, 2018

3 checks passed

codeclimate 3 fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger All green. Jolly good show.
Details

@wafflebot wafflebot bot removed the review-me label Jul 23, 2018

@jywarren

This comment has been minimized.

Contributor

jywarren commented Jul 23, 2018

Great! When you think it's ready, can you start putting the ready label on it? Thanks and great work!

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Jul 24, 2018

great, done, thanks for the help!

@stefannibrasil stefannibrasil deleted the milaaraujo:srch-api-scope branch Jul 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment