NREL / floorspace.js

Other
68 stars 36 forks source link

Add validate schema test #252

Closed bgschiller closed 6 years ago

bgschiller commented 6 years ago

the floorplan exported should satisfy the geometry_schema.

Right now, we're getting these errors:

Should be a number:

Should be a string:

For the "should be a number"s, I believe null is actually appropriate. The description for each says "If provided, ..." I recommend changing the schema to allow null for these values.

For face_id, two choices are possible. We could say "Any space or shading without geometry shall not be exported". However, this would mean that changes to names and other properties would be lost. Alternatively, we could allow face_id to also be null here.

This is a fairly limited test. It would likely make sense to also test some more complicated floorplans, but this is a good start.

macumber commented 6 years ago

@bgschiller agree that all the height's should allow nulls, I suppose that if we are going to automatically create a space and shading object for each story with no face then we should allow face_id to be null as well. I don't really like that, but the alternative is to require a face to create a space which seems cumbersome.

macumber commented 6 years ago

@bgschiller and I decided to make fields required but allow null, rather than making fields optional or optional and allowing null, previously we had a mix of this. The editor tests will ensure that files written conform to this new schema (we need some more complicated test files written out to fully test this). I am not sure we have everything exactly right now but it is more consistent. I allowed fields to only be one type or null, not multiple types for simplicity on reading this file. For bool fields I allowed null which can be a little odd.

macumber commented 6 years ago

I think the schema needs more documentation and default values where appropriate

macumber commented 6 years ago

@bgschiller should project->transform be written to application instead? I see application as being for internal application use, project can be read/written by external applications.

bgschiller commented 6 years ago

@macumber that makes sense, I can move it to application.

Actually, I believe it no longer needs to live in the state at all. (We used to use it to communicate zoom information between the svg layer and the canvas image layer, but we now render images in the svg).

bgschiller commented 6 years ago

Down to the last couple of cases where the export differs from the schema.

Also, I updated the assignable properties to default handle: null, but it's not currently exposed in the UI. Should it be? What does it represent?

@macumber Thanks for working with me on this.