noflo / noflo

Flow-based programming for JavaScript
https://noflojs.org/
MIT License
3.5k stars 263 forks source link

Explicit consistency in components and ports case in .fbp syntax #100

Closed DjebbZ closed 10 years ago

DjebbZ commented 11 years ago

Yeah, it might look like un-important, but at least it is to a beginner like me.

In the noflo website, Getting Started page, libraries are all lowercased (like core and math), components are CamelCased (like Output and ReadFile), ports are uppercased (like IN and OUT). In the cli, noflo list . shows libraries lowercased (ok), components CamelCased (ok), and ports lowercased (huh?).

I suggest the cli output should match the fbp syntax (ports uppercased), and all this case stuff should be documented somewhere. What do you think ?

bergie commented 11 years ago

Agreed, it should be:

trustmaster commented 10 years ago

How about Flowhub and interacting with other runtimes? Should port names be always uppercase? Or lowercase/camelCase and uppercase in FBP only? I've encountered case-sensitivity problems while using Flowhub and implementing GoFlow runtime (which is case-sensitive).

paulyoung commented 10 years ago

Very good point. It seems like ComponentLoader may be a better place to do this.

paulyoung commented 10 years ago

The CLI currently relies on ComponentLoader::listComponents for its output. Internally ComponentLoader::load relies on the same instance variable so perhaps loading would need to become case-insensitive (i.e. convert to lowercase on both sides when matching strings).

paulyoung commented 10 years ago

I introduced a new method since using the existing one complicated things when loading components.

It seems odd to me that this is part of the ComponentLoader class.

paulyoung commented 10 years ago

If anyone can offer better insight, I'm happy to go a different direction.

trustmaster commented 10 years ago

Seems like a subject for another FBP mailing list dispute. I personally prefer case sensitive camelCase port names as it leaves less space for accidents and supports multiWordNames if necessary.

paulyoung commented 10 years ago

When the tests were initially failing, READDOCUMENT did look a bit weird.

subtleGradient commented 10 years ago

I vote for camelCasePortNames.

trustmaster commented 10 years ago

This is my proposal for this issue:

rhalff commented 10 years ago

I experience the same issue using the fbp protocol.

If you make the lowercase decision it basically means all systems talking to the UI are forced to use/send lowercase port names.

Although it is possible to work around this by looking up port names every time one receives a payload and then restore the case sensibility, it does lead to a lot of added complexity to whatever system is trying to talk to the UI.

I think the impact of changing this seems bigger than it probably is.

A grep using toLowerCase() will probably reveal most cases:

$ grep toLowerCase ../noflo-ui/components -r  (Irrelevant lines remove)
../noflo-ui/components/noflo-noflo/src/lib/Graph.coffee:    publicPort = publicPort.toLowerCase()
../noflo-ui/components/noflo-noflo/src/lib/Graph.coffee:    publicPort = publicPort.toLowerCase()
../noflo-ui/components/noflo-noflo/src/lib/Graph.coffee:    publicPort = publicPort.toLowerCase()
../noflo-ui/components/noflo-noflo/src/lib/Graph.coffee:        graph.addInitialIndex conn.data, conn.tgt.process, conn.tgt.port.toLowerCase(), conn.tgt.index, metadata
../noflo-ui/components/noflo-noflo/src/lib/Graph.coffee:      graph.addInitial conn.data, conn.tgt.process, conn.tgt.port.toLowerCase(), metadata
../noflo-ui/components/noflo-noflo/src/lib/Graph.coffee:      graph.addEdgeIndex conn.src.process, conn.src.port.toLowerCase(), conn.src.index, conn.tgt.process, conn.tgt.port.toLowerCase(), conn.tgt.index, metadata
../noflo-ui/components/noflo-noflo/src/lib/Graph.coffee:    graph.addEdge conn.src.process, conn.src.port.toLowerCase(), conn.tgt.process, conn.tgt.port.toLowerCase(), metadata
../noflo-ui/components/noflo-noflo/src/components/Graph.coffee:    return (nodeName+'.'+portName).toLowerCase()
../noflo-ui/components/noflo-noflo/src/components/Graph.coffee:    return (nodeName+'.'+portName).toLowerCase()
../noflo-ui/components/noflo-fbp/lib/fbp.js:          result0 = (function(offset, portname) {return portname.join("").toLowerCase()})(pos0, result0[0]);
../noflo-ui/components/noflo-fbp/lib/fbp.js:          result0 = (function(offset, portname, portindex) {return { port: portname.join("").toLowerCase(), index: parseInt(portindex.join('')) }})(pos0, result0[0], result0[2]);
../noflo-ui/components/noflo-fbp/lib/fbp.js:          parser.exports.push({private:priv.toLowerCase(), public:pub.toLowerCase()})
../noflo-ui/components/noflo-fbp/lib/fbp.js:          parser.inports[pub.toLowerCase()] = {process:node, port:port.toLowerCase()}
../noflo-ui/components/noflo-fbp/lib/fbp.js:          parser.outports[pub.toLowerCase()] = {process:node, port:port.toLowerCase()}
$ grep toLowerCase elements/*
elements/noflo-context.html:            return portName.replace(/(.*)\/(.*)(_.*)\.(.*)/, '$2_$4').toLowerCase();
$ grep toLowerCase preview/ -r
preview/components/forresto-noflo-seriously/lib/SeriouslyEffect.coffee:      nofloPort = seriouslyPort.toLowerCase()

The latter seems to indicate all components should then also be checked, but it also shows how the seriouslyEffect component has to do the same conversion.

https://github.com/forresto/noflo-seriously/blob/master/lib/SeriouslyEffect.coffee#L4

bergie commented 10 years ago

In fd9494fece38de30ef9feb0a438df60fdcb49514 I've now added port name validation to NoFlo. Ports must match the following regexp:

/^[a-z0-9_\.]+$/

trustmaster commented 10 years ago

Good, this will at least avoid collisions in the FBP protocol, where most runtimes would expect port names to be case-sensitive.