Skip to content
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

Ladybug Geometry 'dictutil' converter function #311

Merged
merged 6 commits into from
Aug 6, 2021
Merged

Ladybug Geometry 'dictutil' converter function #311

merged 6 commits into from
Aug 6, 2021

Conversation

ed-p-may
Copy link
Contributor

Adds a new converter function to ladybug_geometry. Similar to the existing 'dictutil' for Honeybee. Takes in a dict and converts to back Ladybug Geometry Object (reverses the .to_dict method). Includes unit tests.

New "dictutil" for Ladybug Geometry, similar to the existing Honeybee "dictutil". Converts any input dict into a new Ladybug Object.
Tests for new Ladybug Geometry dictutil converter function
@CLAassistant
Copy link

CLAassistant commented Jul 29, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ PH-Tools
❌ edpmay
You have signed the CLA already but the status is still pending? Let us recheck it.

@chriswmackey chriswmackey self-assigned this Jul 30, 2021
@chriswmackey chriswmackey added the enhancement New feature or request label Jul 30, 2021
@chriswmackey chriswmackey self-requested a review July 30, 2021 20:23
Update to PR #311 - revised "test/dictutil_test.py" import statements, updated names (files, function) as per comments. Added new tests for completeness. Verified that all tests pass.
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.

Sorry that it took me a while to review this, @PH-Tools . There were just a few styling issues that would be good to fix before we merge. Once they're addressed we can merge.

ladybug_geometry/dictutil.py Outdated Show resolved Hide resolved
ladybug_geometry/dictutil.py Outdated Show resolved Hide resolved
ladybug_geometry/dictutil.py Outdated Show resolved Hide resolved
ladybug_geometry/dictutil.py Show resolved Hide resolved
ladybug_geometry/dictutil.py Outdated Show resolved Hide resolved
@ed-p-may
Copy link
Contributor Author

ed-p-may commented Aug 5, 2021

Great - thanks @chriswmackey ! I appreciate the detailed notes - apologies for the mistakes there. I have updated everything as per your comments and verified that all tests are still passing. I've revised this commit with the new code. Let me know if there is anything else though or if I did anything incorrectly with the update? Happy to revise.

thanks!
@PH-Tools

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 the quick turnaround and definitely no need to apologize. Code review is just part of the process. Thanks again for getting this together.

Merged!

@chriswmackey chriswmackey merged commit 9f8d411 into ladybug-tools:master Aug 6, 2021
@github-actions
Copy link

github-actions bot commented Aug 6, 2021

🎉 This PR is included in version 1.23.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ed-p-may
Copy link
Contributor Author

ed-p-may commented Aug 6, 2021

Great! thanks for helping me get this PR sorted. Glad it worked.
@PH-Tools

@chriswmackey
Copy link
Member

NP. Welcome to the contributor's list!

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

Successfully merging this pull request may close these issues.

4 participants