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

API for fetching nearby coordinates information #1978

Closed
wants to merge 3 commits into from
Closed

API for fetching nearby coordinates information #1978

wants to merge 3 commits into from

Conversation

grvsachdeva
Copy link
Member

Fixes #1934
API working.Suggestions regarding naming and comments are welcome.

@grvsachdeva
Copy link
Member Author

When correct format is followed in URL
first

When correct format not followed
second

@grvsachdeva grvsachdeva reopened this Jan 12, 2018
@@ -182,4 +182,29 @@ def textSearch_questions(srchString)
end
sresult
end

def search_nearbyLocations(srchString)
Copy link
Member

Choose a reason for hiding this comment

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

This is looking great! Could we call this nearbyNodes?

Copy link
Member Author

Choose a reason for hiding this comment

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

doing changes now

@@ -115,6 +115,30 @@ class Search < Grape::API
sresult
end

# Request URL should be /api/srch/locations?srchString=QRY[&seq=KEYCOUNT&showCount=NUM_ROWS&pageNum=PAGE_NUM]
Copy link
Member

Choose a reason for hiding this comment

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

Super! Perhaps we can add this also to /doc/API.md?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link

@asahu8 asahu8 left a comment

Choose a reason for hiding this comment

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

Curious to know. With those changes coming in, can't be have their unit tests/specs written along with it ? Just to ensure this code changes won't break anything if we have the right amount of unit tests in place. @jywarren any thoughts?

coordinates = srchString.split(",")
lat = coordinates[0]
lon = coordinates[1]

Copy link

Choose a reason for hiding this comment

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

latitude, longitude = coordinates
directly we can assign, ruby will take care of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are absolutely right but I think for sake of clarity of all writing one extra line is good

Copy link
Member

Choose a reason for hiding this comment

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

Good suggestion! I always forget to do this myself.

.collect(&:nid)

nids = nids || []

Copy link

Choose a reason for hiding this comment

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

nids ||= []

Copy link
Member

Choose a reason for hiding this comment

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

:-)

get :locations do
sresult = DocList.new
unless params[:srchString].nil? || params[:srchString] == 0 || !(params[:srchString].include? ",")
sservice = SearchService.new
Copy link

Choose a reason for hiding this comment

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

params[:srchString].nil? || params[:srchString] == 0
this particular code is kind of used in multiple places in this file. can't be move it to a private method and re-use it relevant place ?

Copy link
Member

Choose a reason for hiding this comment

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

I think so -- but perhaps independently from this PR so we address code repetition as its own issues. Would you mind opening a new issue to address this? I agree that the services and api code in particular can be enormously compacted without loss of organization. Thank you!

@grvsachdeva
Copy link
Member Author

jeff could you plz help with the build logs
build

@jywarren
Copy link
Member

hmm, this is odd, no? let me take a look.

@jywarren
Copy link
Member

Cannot find a merge base between danger_base and danger_head -- well, searching for this while restarting the build.

@jywarren
Copy link
Member

danger/danger#768

@jywarren
Copy link
Member

oh, do you have two branches of almost the same name?

@jywarren
Copy link
Member

Tough one -- it says the name collision may originate in a different PR -- scary! I'll watch if other PR's travis tests are failing...

@grvsachdeva
Copy link
Member Author

same logs again

@jywarren
Copy link
Member

:-/ i'm not sure about this one! @publiclab/reviewers can anyone help debug this?

@grvsachdeva
Copy link
Member Author

grvsachdeva commented Jan 12, 2018

There is warning about missing secret_key_base too.Do you think it has any role in this ?

@jywarren
Copy link
Member

jywarren commented Jan 12, 2018 via email

@grvsachdeva grvsachdeva deleted the refresh_map branch January 12, 2018 23:56
@grvsachdeva
Copy link
Member Author

Jeff error was generated due to the wrong merging of code on my master branch.Its resolved now.Thanks

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

3 participants