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

Foursquare venue query node and tests (issue #42) #56

Merged
merged 1 commit into from
Nov 3, 2014

Conversation

hbeeken
Copy link
Contributor

@hbeeken hbeeken commented Oct 30, 2014

This node can be merged as it enables the user to explore "food", "outdoor venues" and "sights" near the provided location (lat/lon coordinates). It returns the first recommended venue from the ordered list as determined by Foursquare.

There are of course extensions which can be made (not limiting to 3 categories of venues, not limiting results returned to just the first recommended venue) but these can be done at a later time in other pull requests.

(Part of issue #42)

@coveralls
Copy link

Coverage Status

Coverage increased (+3.6%) when pulling 716b5d9 on hbeeken:foursquare-venue-basic into b53844e on node-red:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.6%) when pulling 39a85f1 on hbeeken:foursquare-venue-basic into b53844e on node-red:master.

this.on("input", function(msg) {
if (msg.lat && msg.lon) {
// if data is in the msg input then this overwrites the node settings
if (msg.section &&
Copy link
Member

Choose a reason for hiding this comment

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

The conclusion reached in node-red/node-red#399 is that if a property is set in the node, it cannot be overridden by the message. We obviously didn't do a good job communicating that around the team.

I'll review the other web nodes to make sure this is followed elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I didn't appreciate that. I followed the behaviour in the weather node and this does the same thing:

    //if there is data in the message input, it overwrites the node setting values.
    //If the data is erroneous or not there, the values remain the node settings.

(weather.js line 20&21)

@zobalogh
Copy link
Contributor

@knolleary Are you planning to include @hbeeken 's changes in the upcoming release too? Do you want me to move the location data under msg.location in this pull request as well? It's @hbeeken's work so I'd rather not touch it, especially as I'm not sure if I can somehow add changes on top of this pull request.

@hbeeken, depending on what @knolleary does, this pull request might need to be updated so that msg.lat, msg.lon is moved under a location object such as msg.location.lat, msg.location.lon.

@knolleary
Copy link
Member

@zobalogh my comment here identified an issue with this PR that needs fixing before it can be merged. The lat/lon change can also be made along with that fix before I merge it.

Will hold the release to next week until this is done - no major urgency to have it done today.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.31%) when pulling d1900b6 on hbeeken:foursquare-venue-basic into 19b83c6 on node-red:master.

@hbeeken
Copy link
Contributor Author

hbeeken commented Nov 3, 2014

This change now includes both the updates to msg.location.lon/msg/location.lat as well as ensuring any node settings override those sent in the incoming msg. I believe it's ready to be merged.

Any updates returning multiple msgs etc. will be done as a separate pull request and build on this one.

knolleary added a commit that referenced this pull request Nov 3, 2014
Foursquare venue query node and tests (issue #42)
@knolleary knolleary merged commit 591e46c into node-red:master Nov 3, 2014
@hbeeken hbeeken deleted the foursquare-venue-basic branch November 12, 2014 10:25
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.

4 participants