graph exports - in / out ambiguity #118

Closed
forresto opened this Issue Nov 18, 2013 · 21 comments

Comments

Projects
None yet
4 participants
Owner

forresto commented Nov 18, 2013

Currently the graph json represents all graph exports with public (component_id.port_name) and private (name of port when shown as subgraph) attributes. This opens up ambiguity if a component has an inport and outport named the same, for example data.

We should fix this ambiguity before it crops up in the wild. I'd vote for separate imports / exports arrays.

forresto referenced this issue in flowhub/the-graph Nov 20, 2013

Closed

Show graph exports #44

Owner

bergie commented Dec 1, 2013

There is some relation also with this and #98

Owner

jonnor commented Dec 1, 2013

Agreed with explicit arrays but I'd be wary of using the names "imports" and "exports". "inPorts" and "outPorts" would fit the semantics better. If it is imaginable that other things can be exported, perhaps have "exports": { "inports": [], "outports": []} inside the graph JSON.

Owner

bergie commented Dec 1, 2013

@jonnor that doesn't sound too bad. However, this would have to be carried also to the FBP parser, as currently there we only recognize this syntax:

EXPORT=SPLIT.IN:IN

We could think of supporting the same syntax, but with keys INPORT and OUTPORT instead of EXPORT. But the old syntax also has to remain, since there are tons of graphs out there using it.

Owner

jonnor commented Dec 28, 2013

Implementing subgraph components in MicroFlo now, and hit this snag (not a showstopper though).
My current thinking is that the EXPORT/INPORT/OUTPORT=foo syntax could be a general attribute/annotation when normalized to JSON.

Owner

bergie commented Jan 2, 2014

We could use the metadata property of graph exports for expressing directionality when needed. The bigger issue is that we need this supported in noflo/fbp

jpaulm commented Jan 7, 2014

I'm pretty sure I raised an issue a few months ago about why the process name in EXPORT has to be in upper case. If this is still true it could seriously impact (natural) languages that use diacritic marks. I am not sure where that issue is, and whether it has been resolved.

Owner

forresto commented Feb 17, 2014

Maybe port names will be ok with letters other than a-z?

"日本語".toLowerCase()
// "日本語"
"ÄÄÄ".toLowerCase()
// "äää"
Owner

forresto commented Feb 17, 2014

Would be nice if we didn't have to .split('.') the private to parse the JSON, and separate the node and port. #140

Owner

bergie commented Feb 17, 2014

@forresto I'm fine with separating private node and port, in which case only port name needs to be lowercased. Do you want to work on that?

Note however that we need to keep backwards compatibility in a way that old FBP and JSON files still work. Easiest would probably be to check for the old . syntax at loadJSON and convert when needed.

Owner

forresto commented Feb 18, 2014

So in loadJSON

if graph.exports instanceof Array
  # old style
else 
  # it should be an object with .inports and .outports arrays

And toJSON() will export that format:

exports: {
   inports: [
     {
       public: 'in',
       process: 'nodeID',
       port: 'portid',
       metadata: {x:0,y:0}
     }
   ],
   outports: []
}

Look good?

forresto closed this in ebb6de9 Feb 18, 2014

Owner

bergie commented Feb 18, 2014

For easier backwards compatibility we should keep the old-style "mixed" exports in the exports array, and then have separate outports and inports arrays for the new ones. We can even make the Graph component reassign the mixed ones when it finds them, as that is the first place where we can really know the direction.

bergie reopened this Feb 18, 2014

@bergie bergie added a commit to flowbased/fbp that referenced this issue Feb 18, 2014

@bergie bergie Parse INPORTS and OUTPORTS, refs noflo/noflo#118 79bc495
Owner

bergie commented Feb 18, 2014

While we're fiddling with this, I think inports and outports should be objects keyed by the published port, since there can be only one port with a given name anyway.

bergie closed this in 494d228 Feb 18, 2014

@forresto forresto added a commit that referenced this issue Feb 18, 2014

@forresto forresto in/out typo #118 eb24633

jpaulm commented Feb 18, 2014

Sorry, I'm having trouble with email on the tablet, and I don't have my Git password!  I'll have a broader bandwidth when I get home.

It seems that the port  name case problem is an artifact of JavaScript, as, in all my implementations of FBP, port names are keywords used for communication between processes and the network.  I have raised the case issue before, without getting an answer - or maybe I didn't understand the answer!  JavaFBP and DrawFBP have no problem supporting Chinese, as port names and process names are just character strings.  Why can't JS handle them?!

I have quite a bit of experience with natural language support,  and I find this is a bit of a blind spot on the part of English-speakers!  The moment you get into languages with diacritics you start to get problems. The Chinese example just makes the problem more obvious.   I have no idea what lowercasing does to Chinese, but if it does anything at all it can't be good!

You might get away with it on statistical grounds as the number of port names to be distinguished should be small for a given component,  but don't be surprised if weird things happen!

I know it's hard to change old architectural decisions, but think of this as red pill time!!!!

Regards, Morpheus  : - )

Sent from Samsung tablet

-------- Original message --------
From: Henri Bergius notifications@github.com
Date: 02-17-2014 3:49 PM (GMT-04:00)
To: noflo/noflo noflo@noreply.github.com
Cc: jpaulm paul.morrison@rogers.com
Subject: Re: [noflo] graph exports - in / out ambiguity (#118)

@forresto I'm fine with separating private node and port, in which case only port name needs to be lowercased. Do you want to work on that?

Note however that we need to keep backwards compatibility in a way that old FBP and JSON files still work. Easiest would probably be to check for the old . syntax at loadJSON and convert when needed.


Reply to this email directly or view it on GitHub.

jpaulm commented Feb 18, 2014

Just started wondering if port names in NoFlo are being treated as variable
names, either in components or in networks, or both...? Can someone point
me at an actual example?
On 2014-02-18 4:12 PM, "jpaulmorr" jpaulmorr@gmail.com wrote:

Sorry, I'm having trouble with email on the tablet, and I don't have my
Git password! I'll have a broader bandwidth when I get home.

It seems that the port name case problem is an artifact of JavaScript,
as, in all my implementations of FBP, port names are keywords used for
communication between processes and the network. I have raised the case
issue before, without getting an answer - or maybe I didn't understand the
answer! JavaFBP and DrawFBP have no problem supporting Chinese, as port
names and process names are just character strings. Why can't JS handle
them?!

I have quite a bit of experience with natural language support, and I
find this is a bit of a blind spot on the part of English-speakers! The
moment you get into languages with diacritics you start to get problems.
The Chinese example just makes the problem more obvious. I have no idea
what lowercasing does to Chinese, but if it does anything at all it can't
be good!

You might get away with it on statistical grounds as the number of port
names to be distinguished should be small for a given component, but don't
be surprised if weird things happen!

I know it's hard to change old architectural decisions, but think of this
as red pill time!!!!

Regards, Morpheus : - )

Sent from Samsung tablet

-------- Original message --------
From: Henri Bergius notifications@github.com
Date: 02-17-2014 3:49 PM (GMT-04:00)
To: noflo/noflo noflo@noreply.github.com
Cc: jpaulm paul.morrison@rogers.com
Subject: Re: [noflo] graph exports - in / out ambiguity (#118)

@forresto https://github.com/forresto I'm fine with separating privatenode and port, in which case only port name needs to be lowercased. Do you
want to work on that?

Note however that we need to keep backwards compatibility in a way that
old FBP and JSON files still work. Easiest would probably be to check for
the old . syntax at loadJSON and convert when needed.

Reply to this email directly or view it on GitHubhttps://github.com/noflo/noflo/issues/118#issuecomment-35316833
.

Owner

bergie commented Feb 18, 2014

@jpaulm character support isn't directly connected with this issue, but anyway...

NoFlo itself doesn't care how the nodes and ports are named, except for the fact that we do normalization via toLowerCase().

The character limitations are mostly coming from the FBP parser where ports are recognized with the regular expression [A-Z0-9_]+.

This limits ports to alphanumeric and uppercase only (on FBP language level, not in NoFlo core). This is bigger limitation than regular JavaScript identifiers which allow unicode characters.

However, as we've discussed before, I'd rather have people target an international community where ASCII characters are pretty much the standard everybody is able to read and write. If we start allowing anything from UTF-8, we risk ending up in a situation where a Chinese programmer can't contribute to a Finnish FBP program and vice versa. I'd rather not fragment the FBP community in that way, at least yet.

Owner

forresto commented Feb 18, 2014

i18n would be cool on the library level, to replace component/port
names/descriptions. Scratch does something like that, and it's cool that
8-year-olds can see the concepts in their own languages.

Owner

bergie commented Feb 18, 2014

@forresto @jpaulm can we get another issue for the i18n question? :-)

@forresto forresto added a commit to flowhub/the-graph that referenced this issue Feb 18, 2014

@forresto forresto refactoring exports noflo/noflo#118 bf5354f

jpaulm commented Feb 18, 2014

@bergie Valid point, Henri! But I agree with @forresto also! Bit of a
catch-22!

And I guess port names are case - insensitive also. That is different from
classic FBP, but just so it is made very clear up front!
On 2014-02-18 4:28 PM, "Henri Bergius" notifications@github.com wrote:

@jpaulm https://github.com/jpaulm character support isn't directly
connected with this issue, but anyway...

NoFlo itself doesn't care how the nodes and ports are named, except for
the fact that we do normalization via toLowerCase().

The character limitations are mostly coming from the FBP parser where
ports are recognized with the regular expression [A-Z0-9_]+.

This limits ports to alphanumeric and uppercase only (on FBP language
level, not in NoFlo core). This is bigger limitation than regular JavaScript
identifiers http://mathiasbynens.be/notes/javascript-identifiers which
allow unicode characters.

However, as we've discussed before, I'd rather have people target an
international community where ASCII characters are pretty much the standard
everybody is able to read and write. If we start allowing anything from
UTF-8, we risk ending up in a situation where a Chinese programmer can't
contribute to a Finnish FBP program and vice versa. I'd rather not fragment
the FBP community in that way, at least yet.

Reply to this email directly or view it on GitHubhttps://github.com/noflo/noflo/issues/118#issuecomment-35429410
.

jpaulm commented Feb 19, 2014

Good idea, Henri! I don't have Git access right now - could someone raise
it?

Actually, I think Henri's point is correct for port names, but I assume
this reasoning only applies to port names, not process names - they should
simply allow all characters, no?

Clsssic FBP does not fold port names and it may be too late to do this now.
Comments?

I also now think I over-constrained process names in DrawFBP - to alpha,
numeric, and underscore only. Unfortunately, I don't remember why. Could
it be because of NoFlo?

Regards,

Paul
On 2014-02-18 6:41 PM, "Henri Bergius" notifications@github.com wrote:

@forresto https://github.com/forresto @jpaulmhttps://github.com/jpaulmcan we get another issue for the i18n question? :-)

Reply to this email directly or view it on GitHubhttps://github.com/noflo/noflo/issues/118#issuecomment-35432082
.

jpaulm commented Feb 20, 2014

"I also now think I over-constrained process names in DrawFBP - to alpha,
numeric, and underscore only."

I now remember why I did this - it had to do with .fbp notation. The logic
goes like this:

In .fbp notation, neither process names nor port names are quoted. This is
not really a problem in the case of port names, as they are normally alpha,
with only square brackets being syntactically significant.

However, if you allow any characters in process names, you have to allow
for the fact that the following characters are all syntactically
significant: comma, hyphen (in arrow), round brackets, blanks, EOL and
semicolon (in Wayne's verson). We can't just surround the process name with
quotes as this is the convention for IIPs.

Note that this problem only applies to .fbp notation - not to any of the
compilable notatons.

In DrawFBP I allow the process name to include special characters, but then
I convert them to underscores, and then of course I have to check for
uniqueness, and make them unique if they are not! Bit of a pain, besides
making the resulting .fbp code look ugly!

How do you all feel about allowing special characters in process names, but
requiring them to be preceded by an escape character - say backslash?

Looking forward to hearing your thoughts on this!

jpaulm commented Mar 1, 2014

Hi guys, I don't see an answer on this one, raised 9 days ago - any comments, anyone?!

Basically, how about adding backslashes before any special characters in process names?

TIA

@KevinHoward KevinHoward added a commit to KevinHoward/noflo that referenced this issue Jun 30, 2014

@forresto @KevinHoward forresto + KevinHoward refactored exports, fixed tests, and added test for legacy exports to…
… loadJSON; fixes #118 #140
259122d

@KevinHoward KevinHoward added a commit to KevinHoward/noflo that referenced this issue Jun 30, 2014

@forresto @KevinHoward forresto + KevinHoward fix splicing while iterating through array #118 20a146d

@KevinHoward KevinHoward added a commit to KevinHoward/noflo that referenced this issue Jun 30, 2014

@bergie @KevinHoward bergie + KevinHoward Handle both the modern in/outports system, and the legacy exports sys…
…tem. Fixes #118
a47dcd3

@KevinHoward KevinHoward added a commit to KevinHoward/noflo that referenced this issue Jun 30, 2014

@forresto @KevinHoward forresto + KevinHoward in/out typo #118 f5591c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment