search

ladybug-tools / honeybee-radiance-command

🐝 ⚡️ :abc: Honeybee wrapper around Radiance commands which is used by honeybee-radiance
https://www.ladybug.tools/honeybee-radiance-command/docs/
GNU Affero General Public License v3.0
0 stars 6 forks source link

feat(Command): Added a few methods to the command class to check inputs. #207

Closed ghost closed 2 years ago

ghost commented 2 years ago

Hi @mostaphaRoudsari ,

  1. I was hoping to get a discussion started so I did not clean up the script for stylistic/PEP-8 type issues (sorry!).
  2. I think the typing module in the same repository would be a better home for these checks. (We can place them in honeybee-core too as you pointed out with your link, although I don't know if they will be useful outside of Radiance).
  3. As to why these might be useful. Mostly for much code-reuse and better maintenance. Specifically:
    • Literally every case of setting the CommandOptions is essentially a copy paste of the same logic. Like here, here, here, here, etc. We are just checking if an instance of 'something' and then instantiating a new instance of that 'something' in case the input is None. Which is what I am doing with the instance_checker method.. I think we'd be able to use this methods for other types of instance checking beyond CommandOptions as well.
    • The reason for adding file checkers are similar as well. Way to much duplication of logic in existing code and future code.

I will delete this branch and send a fresh PR for the typing module.

ghost commented 2 years ago

@mostaphaRoudsari I have incorporated the changes as discussed yesterday in the typing module instead and sent a PR for that: https://github.com/ladybug-tools/honeybee-radiance-command/pull/209. I am going to close this PR now.