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

Deals with csv and json payload in a smarter way #43

Merged
merged 3 commits into from
Apr 16, 2014

Conversation

henols
Copy link
Contributor

@henols henols commented Mar 20, 2014

Deals with csv and json payload in a smarter way and possible manipulation insertion time

Thanks to @PaulRead for pointing out the lack of csv support.

@knolleary
Copy link
Member

Reading through the help, it has gotten quite complicated in terms of what overrides what and when. I'll take another look tomorrow and see if I, as someone who isn't overly familliar with emoncms, can suggest a way to simplify it.

But thanks for your work on this!

@henols
Copy link
Contributor Author

henols commented Mar 21, 2014

He he i know it got a bit complicated. Im really glad if you can look at it or maybe we can change some of the behavior to make it simpler.

@henols
Copy link
Contributor Author

henols commented Mar 31, 2014

@knolleary have you hade time to look at the help text jet? Maybe shall we use the first approach where you select json/csv to make the help text abit simpler.

@knolleary
Copy link
Member

Going to add some comments inline

@@ -48,6 +56,7 @@
defaults: {
name: {value:"Emoncms"},
emonServer: {type:"emoncms-server", required:true},
payloadType: {value:"json"},
Copy link
Member

Choose a reason for hiding this comment

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

If we're autodetecting the payload type, this isn't needed

@knolleary
Copy link
Member

I'm trying to find documentation on the emoncms rest api that this node uses, to better understand the difference between using cvs and json. I can't find anything at that level of detail in their docs - have you got a link to anything useful?

@henols
Copy link
Contributor Author

henols commented Apr 1, 2014

First I'm sorry but I think I was a bit quick to check in the code so I missed payloadType.
I think I wrote to someone or somewhere: Since English, GUI, usability and User documentation isn't my strongest skills. Or just I misunderstood what went where, thats why the tip got to long.

The main reason to use json is that you can provide your own key and in csv the keys will be generated by emoncms.
Some documentation can be found here:
http://emoncms.org/site/docs/inputsandfeeds
http://emoncms.org/site/docs/developinputproc

While I was reading the documentation I found out the json post is a bit more capable then I first got the impression of. So I think I have to rewrite the node once more and remove the key field that will make the help text a little bit simpler with less overrides and a clearer perspective of how the payload can contain.

Henrik

@knolleary
Copy link
Member

I found those docs, but scanning through them didn't see solid API documentation.

The docs should focus on what the user is trying to do. Something along the lines of:

You can publish a single feed value by...
You can publish multiple feed values by....

I'll hold-off looking any more until you've reworked it as you say you are (unless you decide not to rework it - let me know)

@henols
Copy link
Contributor Author

henols commented Apr 3, 2014

@knolleary now I have rewritten the help text removed the key field and changed the logic a bit.
Please have a look, I think this is the way to go.

Henrik

@knolleary
Copy link
Member

node-red-gitbot: check cla status

@henols - thanks, this looks good. I think there is scope to support msg.payload being an object containing key/value pairs, rather than a string with them encoded in it... but that can be a future enhancement.

knolleary added a commit that referenced this pull request Apr 16, 2014
Deals with csv and json payload in a smarter way
@knolleary knolleary merged commit afa46ef into node-red:master Apr 16, 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.

2 participants