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

feat(altnumber): Add NoLimit, AutoCalculate and AutoSize objects #44

Merged
merged 2 commits into from
Feb 4, 2020

Conversation

chriswmackey
Copy link
Member

This commit addresses this issue here.

The tests on this PR won't pass until I update the core Python libraries and re-export all of the samples. I am working on this now and will amend this PR shortly. In the meantime, @mostaphaRoudsari and @MingboPeng , can you let me know if these changes look good to you and confirm that they satisfy the issues you raised, Mingbo?

@chriswmackey chriswmackey added enhancement New feature or request critical Features/fixes that are critical to continued development labels Feb 3, 2020
@chriswmackey chriswmackey self-assigned this Feb 3, 2020
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.

Looks good to me. Do you think it makes sense to add a value field which returns the value? It will be identical to type but it might be closer to what one would expect. I don't think it is a required change as one can use .type to get the same value during the translation process.

chriswmackey added a commit to chriswmackey/honeybee-core that referenced this pull request Feb 3, 2020
This commit brings the library up-to-date with these recent changes in honeybee-schema:
ladybug-tools/honeybee-schema#44

Only the to_dict methods have been changed and the Python objects still internally use strings for simplicity. An alternative approach could use classes for AutoCalculate, AutoSize and NoLimit and use them similarly to how the `facetype` module is used. A change like this would involve a bunch of edits in a lot of different places, though. @mostaphaRoudsari, what do you think?
@MingboPeng
Copy link
Member

MingboPeng commented Feb 3, 2020

@chriswmackey I think this look great, This is more flexible and future-proofing.
I agree with @mostaphaRoudsari , since these three objects will only have one instance. Is it possible to make these classes static and use .value to get its instance?

@chriswmackey
Copy link
Member Author

@mostaphaRoudsari , I think you might be confusing functionality of classes producing the schema with the schema itself. An extra redundant key in the schema seems like it only opens up the possibility for more typos or illegal combinations of values in the JSONs people submit.

However, I can see the ability to get the "autocalculate" text from an AutoCalculate object as helpful in either Python or C# bindings. Right now, I wasn't thinking to implement a whole new class for these things in the Python bindings but I can change this if you let me know your thoughts here: ladybug-tools/honeybee-core#53

@chriswmackey
Copy link
Member Author

Just to clarify what I meant. Adding redundant keys to the schema seems fine from the perspective of people building schema JSONs thorough C# or Python bindings. But we should also think about people working with or building the JSONs in the absence of these bindings, as is the case with the energy_model_measure, Theo's web viewer, and I am sure many other applications. Having to edit two keys to make a change instead of one is not helpful from this perspective.

chriswmackey added a commit to chriswmackey/honeybee-core that referenced this pull request Feb 4, 2020
This commit brings the library up-to-date with these recent changes in honeybee-schema:
ladybug-tools/honeybee-schema#44
chriswmackey added a commit to chriswmackey/honeybee-energy that referenced this pull request Feb 4, 2020
This commit brings the library up-to-date with these recent changes in honeybee-schema:
ladybug-tools/honeybee-schema#44
chriswmackey added a commit to chriswmackey/honeybee-openstudio-gem that referenced this pull request Feb 4, 2020
This commit brings the measure in line with this recent change in the schema:
ladybug-tools/honeybee-schema#44
chriswmackey added a commit to chriswmackey/honeybee-grasshopper-energy that referenced this pull request Feb 4, 2020
This commit brings the plugin up-to-date with these recent changes in honeybee-schema:
ladybug-tools/honeybee-schema#44
chriswmackey added a commit to chriswmackey/honeybee-grasshopper-energy that referenced this pull request Feb 4, 2020
This commit brings the plugin up-to-date with these recent changes in honeybee-schema:
ladybug-tools/honeybee-schema#44
@chriswmackey chriswmackey merged commit 72a72f5 into ladybug-tools:master Feb 4, 2020
chriswmackey added a commit to ladybug-tools/honeybee-core that referenced this pull request Feb 4, 2020
This commit brings the library up-to-date with these recent changes in honeybee-schema:
ladybug-tools/honeybee-schema#44
chriswmackey added a commit to ladybug-tools/honeybee-openstudio-gem that referenced this pull request Feb 4, 2020
This commit brings the measure in line with this recent change in the schema:
ladybug-tools/honeybee-schema#44
chriswmackey added a commit to ladybug-tools/honeybee-grasshopper-energy that referenced this pull request Feb 4, 2020
This commit brings the plugin up-to-date with these recent changes in honeybee-schema:
ladybug-tools/honeybee-schema#44
@ladybugbot
Copy link
Contributor

🎉 This PR is included in version 1.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

chriswmackey added a commit to chriswmackey/honeybee-energy that referenced this pull request Feb 4, 2020
This commit brings the library up-to-date with these recent changes in honeybee-schema:
ladybug-tools/honeybee-schema#44
chriswmackey added a commit to ladybug-tools/honeybee-energy that referenced this pull request Feb 4, 2020
This commit brings the library up-to-date with these recent changes in honeybee-schema:
ladybug-tools/honeybee-schema#44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical Features/fixes that are critical to continued development enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants