Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(radiance): Adding initial radiance schema materials. #65

Merged
merged 5 commits into from
Apr 14, 2020
Merged

feat(radiance): Adding initial radiance schema materials. #65

merged 5 commits into from
Apr 14, 2020

Conversation

saeranv
Copy link
Member

@saeranv saeranv commented Apr 7, 2020

@chriswmackey

This is a draft of some radiance materials to review if this is the correct approach.

Schemas done:

  • Primitive
  • Plastic
  • Glass
  • Metal

@saeranv saeranv requested a review from chriswmackey April 7, 2020 00:49
@saeranv saeranv self-assigned this Apr 7, 2020
@saeranv saeranv added the enhancement New feature or request label Apr 7, 2020
@saeranv
Copy link
Member Author

saeranv commented Apr 10, 2020

Note, this latest PR is for WIP discussion only, shouldn't be merged in yet.

@mostaphaRoudsari
Copy link
Member

Note, this latest PR is for WIP discussion only, shouldn't be merged in yet.

Create a draft PR or you can now change it to draft:

image

@saeranv saeranv marked this pull request as draft April 10, 2020 02:11
Copy link
Member

@mostaphaRoudsari mostaphaRoudsari left a comment

Choose a reason for hiding this comment

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

Hi @saeranv. See my comments. The recursive referencing should be fine. I shared the link on how you can set it up in Pydantic. Test it and let me know if it works for you.

Copy link
Member

@chriswmackey chriswmackey left a comment

Choose a reason for hiding this comment

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

Thanks for getting this together @saeranv . It's looking good and most comments are minor.

@saeranv
Copy link
Member Author

saeranv commented Apr 11, 2020

@mostaphaRoudsari @chriswmackey

Ready for review round 2.

@mostaphaRoudsari mostaphaRoudsari self-requested a review April 11, 2020 11:38
Copy link
Member

@mostaphaRoudsari mostaphaRoudsari left a comment

Choose a reason for hiding this comment

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

Hi @saeranv, Thank you for updating the PR quickly. Looks good overall but we won't know if the reference idea works until you add tests. Cheers.

@saeranv
Copy link
Member Author

saeranv commented Apr 13, 2020

Hi @saeranv, Thank you for updating the PR quickly. Looks good overall but we won't know if the reference idea works until you add tests. Cheers.

@mostaphaRoudsari, I checked that it works by calling all the json schema in the if __name__ == '__main__': section. It was giving me an 'undefined class' error before, but with the references everything is working.

Do you still want me to add unit tests? In my last discussion with @chriswmackey we decided not to do any tests for now, but can take a look at it if needed.

@saeranv
Copy link
Member Author

saeranv commented Apr 13, 2020

@mostaphaRoudsari, @chriswmackey

Revised, let me know if it's ready to be squashed and merged.

@mostaphaRoudsari
Copy link
Member

Hi @saeranv, Other than tests it looks good to me. Can I ask why did you and @chriswmackey decide not to write tests at this point?

@saeranv
Copy link
Member Author

saeranv commented Apr 13, 2020

Hi @saeranv, Other than tests it looks good to me. Can I ask why did you and @chriswmackey decide not to write tests at this point?

@mostaphaRoudsari nothing specific as I recall. I think it was just because I was just getting introduced to pydantic at that point and we figured we would revisit the question of testing at a later point.

@mostaphaRoudsari
Copy link
Member

I see. Then let's add tests. It will save us in the near future. 😀

@saeranv saeranv marked this pull request as ready for review April 14, 2020 16:56
@chriswmackey
Copy link
Member

I think this is good to squash and merge now.

@mostaphaRoudsari ,
Here is the plan for adding the tests: https://3.basecamp.com/4298486/buckets/13923324/question_answers/2577596456#__recording_2577711674
The reason why we are doing it this way is because I know it will be a lot faster for me to write a bunch of different sample JSONs than it will be for Saeran. Saeran will still write the code that runs the samples through his code and fix any bugs that he finds.

@saeranv saeranv merged commit bbd7257 into ladybug-tools:master Apr 14, 2020
@ladybugbot
Copy link
Contributor

🎉 This PR is included in version 1.22.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants