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

Closed
DjebbZ opened this Issue Aug 13, 2013 · 13 comments

Comments

Projects
None yet
6 participants

DjebbZ commented Aug 13, 2013

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 ?

jonnor added a commit to microflo/microflo that referenced this issue Sep 24, 2013

Lower-case port identifiers for analog pins
NoFlo or the FBP parser normalizes the typical upper-case port
usage to lower case internally, which would not match port definitions
of form "pinA1" as we do not normalize on our side.
See also noflo/noflo#100
Owner

bergie commented Oct 10, 2013

Agreed, it should be:

  • Node names (component instances) are CamelCased
  • Port names are UPPERCASE (like in .fbp)

paulyoung added a commit to paulyoung/noflo that referenced this issue May 13, 2014

Owner

trustmaster commented May 13, 2014

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).

Contributor

paulyoung commented May 13, 2014

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

Contributor

paulyoung commented May 13, 2014

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 added a commit to paulyoung/noflo that referenced this issue May 13, 2014

paulyoung added a commit to paulyoung/noflo that referenced this issue May 13, 2014

Contributor

paulyoung commented May 13, 2014

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.

Contributor

paulyoung commented May 13, 2014

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

Owner

trustmaster commented May 13, 2014

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.

Contributor

paulyoung commented May 13, 2014

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

@trustmaster trustmaster referenced this issue in noflo/noflo-ui May 14, 2014

Closed

No support for mixed case port names #234

I vote for camelCasePortNames.

Owner

trustmaster commented Jun 11, 2014

This is my proposal for this issue:

  • Make port names case-sensitive in the FBP protocol (and in NoFlo-UI), use camelCase port names in NoFlo.
  • Use uppercase port names in .fbp files for historical reasons, but infer case-sensitive port names from component registry when loading a graph.

rhalff commented Jul 28, 2014

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

Owner

bergie commented Aug 22, 2014

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

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

@bergie bergie closed this Aug 22, 2014

Owner

trustmaster commented Aug 22, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment