-
Notifications
You must be signed in to change notification settings - Fork 179
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
TheGraph.findMinMax() assumes metadata is numerical but they could be strings #208
Comments
@bergie should we parseInt for certain metadata in fbp? And/or be safe here? |
@davidmaxwaterman could do the |
IMO, the values should be integers as soon as they're read (this sort of error could happen anywhere, no just in findMinMax()), and the fix should go into fbp parser...surely, something can be added to : https://github.com/noflo/fbp/blob/master/grammar/fbp.peg to convert them if they're values that are known to be numeric? Somewhere around Line 32, I expect : https://github.com/noflo/fbp/blob/master/grammar/fbp.peg#L32 I might have a go at getting that to work... |
I should add that there is also an issue with the fbp parser where it lower-cases the port names, and that causes the ports to be duplicated. Why would it lower-case them, I wonder? I've added code to go through them and uppercase them again and it works as expected? Comments? |
I'm using loadFBP() to load a fbp file a bit like the following :
Unfortunately, the x,y, width, and heights are interpreted as strings, eg :
I'm not sure how the fbp parser can determine if they should be strings or not, but TheGraph.findMinMax() adds two of these values together and because they're strings the result is huge.
https://github.com/the-grid/the-graph/blob/master/the-graph/the-graph.js#L110
Perhaps it would be appropriate to put parseInt() around those values?
I'm happy to make a PR...
The text was updated successfully, but these errors were encountered: