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

Barclays Cycle Hire Node #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Barclays Cycle Hire Node #87

wants to merge 1 commit into from

Conversation

Raminios
Copy link
Contributor

Added the barclays cycle hire node. The query node is the intended implementation, the input node is temporary and a full implementation should come after feedback. Feedback/comments welcome!

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 03d19e6 on Raminios:london-cycle-node into 19b1e01 on node-red:master.

@zobalogh
Copy link
Contributor

You'll need to move your nodes into the ./transport directory. See @hbeeken 's work on the underground node and ensure that you follow her conventions. I did the same for my buses node. Whoever gets in first is to be followed by the rest and regard them as new, accepted conventions. ;)

res.on('data', function (chunk) {
data+= chunk;
});
res.on('end', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be prone to errors too, similarly to your forecast.io node that failed when the server didn't respond as expected. This needs investigation or maybe you could proactively catch exceptions here. In the error scenario, the node should report a visible error to the user (node.error). Input nodes should send no messages, a query node shall simply forward the incoming message.

@zobalogh
Copy link
Contributor

zobalogh commented Dec 8, 2014

@Raminios , you'll still need to move the node into the ./transport directory. Also, name it "tfl-cyclehire" or similar. Thanks!

@Raminios
Copy link
Contributor Author

Raminios commented Dec 8, 2014

@zobalogh Both of these changes have been made? If i click the files changed tab I see all files under transport and using the node name tfl-cyclehire. Is it still necessary for the file names to be tfl-cyclehire? I thought them being in that directory would suffice

@zobalogh
Copy link
Contributor

zobalogh commented Dec 8, 2014

@Raminios OK, I looked at my local copy of your repository, sorry. It must be out of date for some reason. I think calling it tfl-cyclehire is better as that name will be included in flows. However this is not "THE" cycle hire node, it's TfL specific.

@Raminios Raminios changed the title [WIP] Barclays Cycle Hire Node Barclays Cycle Hire Node Dec 15, 2014
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.

3 participants