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

Remove API Search endpoints duplication #3045

Merged
merged 4 commits into from Jul 18, 2018

Conversation

Projects
None yet
5 participants
@stefannibrasil
Copy link
Collaborator

stefannibrasil commented Jul 11, 2018

Hello, everyone!

We are opening this PR to get some feedback about our work with the API.

In order to refactor how the searches are being made, we decided to first try to remove some duplications. We have done some small changes but we wanted to know what do you think and see if there is another thing we can remove/change.

@publiclab/reviewers @publiclab/soc @jywarren

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Jul 11, 2018

Hi, this is due to a new check that was added recently to encourage very consistent formatting. I've actually turned off the check until we can make it's output friendlier, but since your work is based on an earlier version of code, you're seeing some formatting suggestions. You can resolve them by going through them one by one here:

https://travis-ci.org/publiclab/plots2/builds/402502945#L3400

Sorry, it's a bit obscure, we're working to make this more readable!!! Thanks!

@stefannibrasil stefannibrasil force-pushed the milaaraujo:refactor-api branch from 2b32937 to 8dc4729 Jul 12, 2018

@plotsbot

This comment has been minimized.

Copy link
Collaborator

plotsbot commented Jul 12, 2018

2 Messages
📖 @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.
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.

Generated by 🚫 Danger

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Jul 12, 2018

Cool! got the tests to pass, i see! Now, CodeClimate is complaining about "cognitive complexity" -- basically, asking you to try to simplify how the code is written in order to make it more readable.

CodeClimate is picky -- any time you think it's being too particular, you can ping me and I can manually "approve" a PR and we can move forward -- not a problem! But if you click "details" next to CodeClimate, it'll offer some advice: https://codeclimate.com/github/publiclab/plots2/pull/3045

If you hover over the right-side of each issue, it has a "read more" link which offers some tips on how to resolve it:

image

These are actually really nice and helpful despite the Xs!

image

For this one, I'm OK with how you've done it. If you'd like, I can just approve it -- just tell me what you think!

Actually, what I just wrote in this comment is a nice thing we could add to a README in the repository... what do you think? I'd accept a PR with this text in a CODECLIMATE.md file... if you're interested! We could point new contributors toward it to help them deal with CodeClimate errors.

Thanks!

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Jul 12, 2018

@mridulnagpal is working on CodeClimate on his own project and may find this interesting!

@stefannibrasil

This comment has been minimized.

Copy link
Collaborator

stefannibrasil commented Jul 13, 2018

HI, Jeff!

Yeah, sorry, I forgot to tell that we fixed the small issues. I didn't say anything because we are still working on this. We are breaking the responsibilities into new classes so we may fix that issue. Just having a little problem trying to understand a Grape error for now! :P

And great idea about the codeclimate.md file! We can start that for sure :)

@milaaraujo

This comment has been minimized.

Copy link
Collaborator

milaaraujo commented Jul 13, 2018

Hey @jywarren, today we worked on our code to remove cognitive complexity from the Search API. We still have to work on minor repairs, but we'll do it tomorrow!

@milaaraujo

This comment has been minimized.

Copy link
Collaborator

milaaraujo commented Jul 13, 2018

Hey @jywarren, it looks like all checks have passed now!

@stefannibrasil

This comment has been minimized.

Copy link
Collaborator

stefannibrasil commented Jul 14, 2018

@publiclab/soc @publiclab/reviewers hey, everyone, just one question regarding the Typeahead Service.
We did the same thing as this PR does to the Search Service but we are not really sure what this does in the Typeahead code:

present sresult, with: TagList::Entity

We run the tests and searched locally the API and we couldn't see any difference in the sresult list between using that or removing it. Can someone please explain more about this? Thank you!

@stefannibrasil stefannibrasil changed the title WIP - Remove API Search enpoints duplication Remove API Search enpoints duplication Jul 14, 2018

@stefannibrasil stefannibrasil changed the title Remove API Search enpoints duplication Remove API Search endpoints duplication Jul 15, 2018

@stefannibrasil stefannibrasil referenced this pull request Jul 15, 2018

Closed

Refactor Search API #3070

3 of 3 tasks complete

@stefannibrasil stefannibrasil removed this from In progress in RGSoC 2018 Jul 15, 2018

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Jul 15, 2018

Hmm, I'm not sure about that either, but can try to trace through the code to figure it out. Can you find documentation for the present keyword? TagList is a class custom built for this codebase by @david-days - maybe he can chime in? Can you link directly to the line of code in question?

@sagarpreet-chadha

This comment has been minimized.

Copy link
Contributor

sagarpreet-chadha commented Jul 16, 2018

Hi @stefannibrasil ...

class Entity < Grape::Entity

In comments above this internal-class is written :
This subclass is used to auto-generate the RESTful data structure. It is generally not useful for internal Ruby usage but must be included for full RESTful functionality.

Hope it helps !

@milaaraujo

This comment has been minimized.

Copy link
Collaborator

milaaraujo commented Jul 16, 2018

Hey @jywarren, @stefannibrasil's question (present keyword) is not directly connect with this PR (#3045). So are still waiting you to approve this one! 😬 And then we can open a new PR for removing API Typeahead endpoints duplication.

@stefannibrasil

This comment has been minimized.

Copy link
Collaborator

stefannibrasil commented Jul 17, 2018

@milaaraujo is right! We are waiting for the review of this PR before opening the Typeahead refactor proposal. We did the same thing as we did here with the Search.

While this one isn't approved, I decided to ask about the Typeahead service. This is the line that I was referring to (we can continue the discussion after this current PR is reviewed/approved):

present sresult, with: TagList::Entity

We wanted to understand why Typeahead adds that to each endpoint call. If we remove it, we don't see any difference in the results format, so we don't understand what exactly that line is supposed to do.

@sagarpreet-chadha do you have an idea why only typeahead use that? the search service doesn't use the DocList::Entity 🤔

And even if we remove that line, we still get the results as a list:

{
  "items": [
    {
      "tagId": 8,
      "tagVal": "Blog Post",
      "tagType": "file",
      "tagSource": "/notes/admin/07-10-2018/blog-post"
    }
  ],
  "srchParams": {
    "srchString": "Blog",
    "seq": null,
    "showCount": null,
    "pageNum": null,
    "tagName": null
  }
}

so there is no apparent difference between using that or not. The result is already a TagList. I have a guess that we don't need that at all, but we wanted to know what do you think 💭

@jywarren
Copy link
Contributor

jywarren left a comment

This is awesome, i love it. Great work on breaking into separate files. I asked for a couple extra comments to help orient people who aren't familiar with how the files are split up. Then I think we're good!

@@ -166,29 +211,4 @@ def app

end

test 'search recent people functionality having specified tagName' do

This comment has been minimized.

@jywarren

jywarren Jul 17, 2018

Contributor

Did you remove this because it's covered by the all test? Just checking!

@@ -3,6 +3,8 @@

module Srch
class Search < Grape::API
helpers SharedParams

This comment has been minimized.

@jywarren

jywarren Jul 17, 2018

Contributor

Should we write a comment like # see /app/api/srch/shared_params.rb ?

extend Grape::API::Helpers

params :common do
requires :srchString, type: String, documentation: { example: 'Spec' }

This comment has been minimized.

@jywarren

jywarren Jul 17, 2018

Contributor

This is fantastic. Thanks!!!

This comment has been minimized.

@stefannibrasil

stefannibrasil Jul 18, 2018

Collaborator

@jywarren we added the test back. We'll probably work more on the tests in the next weeks, but just to be more careful, we think it's best to not remove it for now. Good catch!

we've already done the same strategy here in the Typeahead, we are planning to open a PR for that after this is approved!

Thanks! 💯

@stefannibrasil stefannibrasil force-pushed the milaaraujo:refactor-api branch 2 times, most recently from 32bb9cd to ac12304 Jul 18, 2018

@stefannibrasil stefannibrasil force-pushed the milaaraujo:refactor-api branch from ac12304 to 5b8bcb7 Jul 18, 2018

@milaaraujo

This comment has been minimized.

Copy link
Collaborator

milaaraujo commented Jul 18, 2018

Hi guys, I'm adding a last modification - I hope so! - to the code. recentPeople method should "GET X number of latest people/contributors, X = srchString". So I changed from:

nodes = Node.all.order("changed DESC").limit(100).distinct -> nodes = Node.all.order("changed DESC").limit(srchString).distinct

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Jul 18, 2018

I think the error we're seeing may have been resolved in another PR - so you should be able to rebase your work over the latest master and it should disappear. However I've restarted the build to see if it "resolves itself" first... you can always re-run Travis tests by closing and reopening the PR.

You can rebase by following the process outlined here, but you may be able to skip step 2 if your master is still in sync with publiclab's master: https://publiclab.org/wiki/contributing-to-public-lab-software#Rewinding+the+master+branch

Thanks!

@milaaraujo

This comment has been minimized.

Copy link
Collaborator

milaaraujo commented Jul 18, 2018

It passed now, @jywarren!

@jywarren jywarren merged commit 5c94fe5 into publiclab:master Jul 18, 2018

3 checks passed

codeclimate 12 fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger All green. Well done.
Details

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

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Jul 18, 2018

Hooray!!!! 🎉 🎉 🎉

@milaaraujo milaaraujo referenced this pull request Jul 19, 2018

Closed

Remove API Typeahed endpoints duplication #3096

0 of 5 tasks complete

@stefannibrasil stefannibrasil deleted the milaaraujo:refactor-api branch Jul 21, 2018

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