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

Support anonymous nodes, default ports, JSON IIPs, etc #58

Merged
merged 9 commits into from Jun 17, 2016

Conversation

tlrobinson
Copy link
Contributor

@tlrobinson tlrobinson commented Jun 10, 2016

This PR does a bunch of things. I can try to split them up into multiple PRs but I'd rather not because some of them are interdependent.

You can now write incredibly succinct networks, e.x.

(stdin) -> (uppercase) -> (stdout)

Allow sending other valid JSON types as IIPs

Resolves #10

You can now do things like:

1234 -> IN Display(Output)
"foo" -> IN Display(Output)
[1,2,3,4] -> IN Display(Output)
{ "foo": "bar" } -> IN Display(Output)
{ "foo": { "bar": { "baz": 1234 }}} -> IN Display(Output)

true, false, and null don't work because they are valid node names.

I'm just using the JSON grammar straight from peg.js' example directory.

Allow anonymous nodes

Resolves #11

Nodes without a name are supported. Internally they are given a unique name based on the component name and index to help debugging.

'foo' -> IN (Output)

The Output component is internally named _output_1.

Declare components before connecting them

Resolves #12

Lets you declare a component without connecting it immediately:

Display(Output)
'foo' -> Display

Parser seems to need blanks around the arrows.

Resolves #21

You can now do

(A)->(B)

Default Ports

Resolves #32

The port name can be left off in which case it will default to "IN" or "OUT". The following are equivalent:

(A) -> (B)
(A) OUT -> IN (B)
(A) OUT -> (B)
(A) -> IN (B)

In certain causes this is not allowed due to ambiguity, e.x.

(A) OUT -> X Y -> (C)

Is X the inport and Y the component, or is X the component and Y the outport?

@tlrobinson tlrobinson changed the title Support JSON IIPs, including bare numbers, etc Support anonymous nodes, default ports, and JSON IIPs Jun 13, 2016
@tlrobinson
Copy link
Contributor Author

I went ahead and implemented a bunch of the other enhancements I wanted. Please let me know what I need to do to get some or all of this merged.

@tlrobinson tlrobinson changed the title Support anonymous nodes, default ports, and JSON IIPs Support anonymous nodes, default ports, JSON IIPs, etc Jun 13, 2016
@@ -136,6 +226,28 @@ describe 'FBP parser', ->
chai.expect(graphData.inports).to.be.an 'undefined'
chai.expect(graphData.outports).to.be.an 'undefined'

describe 'with FBP string containing a JSON IIP string', ->
Copy link
Contributor

@jonnor jonnor Jun 14, 2016

Choose a reason for hiding this comment

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

Can you also add a test for couple levels of nested object? It is the kind of thing which can easily break, in my experience

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I forgot about this. It should be fine since i'm using peg.js' own JSON grammar, which I assume has been thoroughly tested, but I can do a followup PR to add a test if you want.

@jonnor
Copy link
Contributor

jonnor commented Jun 17, 2016

Lots of good improvements here. Some quick testing on couple of .fbp heavy projects, and the changes does not seem to break anything, so think we can merge this.

In the future please split up PRs a bit, as it is hard to review such big changes. And sometimes one ends up with scenarios where issues with some parts of the PR blocks inclusion of parts without issues.

@jonnor jonnor merged commit 43bdd2f into flowbased:master Jun 17, 2016
@jonnor
Copy link
Contributor

jonnor commented Jun 17, 2016

Released as fbp 1.4.0. The dependency bump has already been tested by Greenkeeper and merged to noflo, fbp-manifest and noflo-ui.

Thanks @tlrobinson !

@jpaulm
Copy link

jpaulm commented Jun 17, 2016

Sorry, I am still not clear about the arrows: are you saying that you can leave out the surrounding blanks iff the component name is specified (apparently surrounded by brackets)? Are you allowing hyphens in process names? Is there a clear spec for a) process names and b) port names, or do I have to read the regex to figure it out? Either way, are you allowing Chinese characters in process names? I assume not in port names, as per my earlier post about the discussion with Henri.

Just noticed: Humberto Madeira wants hyphens in port names - will that work?

@tlrobinson
Copy link
Contributor Author

@jonnor Thanks! I'll try to split things up more next time.

@jpaulm It seems like node names can include dashes, but if that's the case you can't leave off spaces around the arrows, i.e. x -> y and x()->y() work, but x->y is a parse error (though you can also leave out the spaces if there are port names x out->in y works).

I think in theory you should be able to do x->y since > is not allowed in node names thus it is not ambiguous, but currently the grammar doesn't allow it.

For reference:

Port name:

= portname:([a-zA-Z_][a-zA-Z.0-9_]*) {return makeName(portname)}

Node name:

= name:([a-zA-Z_][a-zA-Z0-9_\-]*) { return makeName(name)}

Component name:

= "(" comp:([a-zA-Z/\-0-9_]*)? meta:compMeta? ")" { var o = {}; comp ? o.comp = comp.join("") : o.comp = ''; meta ? o.meta = meta.join("").split(',') : null; return o; }

@chadrik
Copy link
Contributor

chadrik commented Jun 17, 2016

For supporting "rich" port names (anything other than [a-zA-Z_][a-zA-Z.0-9_]*), I suggest we have a quoted variant that allows more characters.:

1234 -> IN 'Display: Such wow!'(Output)

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.

None yet

4 participants