pollination / queenbee

👑 Queenbee is a workflow language for creating workflows
https://pollination.github.io/queenbee
MIT License
18 stars 5 forks source link

"Union[List, Dict]" breaks the C# SDK generation #260

Closed MingboPeng closed 3 years ago

MingboPeng commented 3 years ago

The most recent fix https://github.com/pollination/queenbee/commit/41fb857c2f2658f390e0eab5a787616266d91791 breaks the C# SDK generation. List is not recognized inside Union (AnyOf) during the translation.

So why do we need make value to be List type when we have DAGArrayInput, DAGArrayInputAlias, FunctionArrayInput, FunctionArrayInputAlias, etc. that are design for lists?

@AntoineDao @mostaphaRoudsari

mostaphaRoudsari commented 3 years ago

This was meant to give more flexibility for writing recipes. @AntoineDao, if you can point me to the task that needed this change I can try to change the recipe to Array type and we can revert this change.

AntoineDao commented 3 years ago

Ah that makes sense, sorry about that @MingboPeng . Can I propose the following steps to fix this:

  1. Reset the JSONObject types to be only Dict
  2. Deprecate (not remove) the JSONObject types in favour of a Dict or Map or Object type. Calling it JSONObject really insinuates that it should support Arrays and Dicts at the same time. I prefer calling it Object type as that is what it's called in the JSON Schema and OpenAPI schema.
  3. Replace the incorrect JSONObject type in the daylight-factor recipe to Array type
  4. Add some more flexible type casting primitives when translating Run Step parameters (ie: if a JSONObject type is actually an Array don't throw an error but instead cast it to an Array type)

Let me know what you guys think and I'll get started on work to fix this ASAP 😀

mostaphaRoudsari commented 3 years ago

Thank @AntoineDao! Can we leave the step 2 for later? It used to be called Object before and it become confusing for @MingboPeng and that's how/why we changed it to JSONObject. I like the proposal and fixing the recipes to work with the current set up should be easy to do. I agree that we should have an object that supports any valid JSON object either if it translates to a dictionary, list or a string in Python.

AntoineDao commented 3 years ago

Sounds like a plan. Is it ok if I fix this tomorrow so I can finish deploying our production environment today?

mostaphaRoudsari commented 3 years ago

Sure! If you give me a lead on where the recipe is broken I can fix it on the recipe side and revert back the changes on queenbee.

mostaphaRoudsari commented 3 years ago

Actually, I think I know where the issue is. It is in the Function itself and not the recipe. This should be set to a list here:

https://github.com/pollination/pollination-honeybee-radiance/blob/master/pollination_honeybee_radiance/grid.py#L21-L24

Assuming Chris has followed a similar pattern I think there is a similar case in the one for energy simulation which I also need to fix. On the luigi side since both of these are parsed from a JSON file it doesn't really make a difference.

AntoineDao commented 3 years ago

Yep happy with either or 😀

ladybugbot commented 3 years ago

:tada: This issue has been resolved in version 1.22.16 :tada:

The release is available on:

Your semantic-release bot :package::rocket: