add MinerConfig support for ePIC UMC #84
Conversation
pyasic/config/mining.py
Outdated
There was a problem hiding this comment.
Maybe wrap all this in a try: except LookupError: instead of using get?
pyasic/config/pools.py
Outdated
|
|
||
| @classmethod | ||
| def from_epic(cls, api_pool: dict) -> "Pool": | ||
| return cls(url=api_pool["pool"].replace("stratum+tcp://", ""), user=api_pool["login"], password=api_pool["password"]) |
There was a problem hiding this comment.
Don't replace the prefix here, want to keep everything for the URL.
Might be possible at a later point to split the URL into its components, but not really worth doing yet.
pyasic/config/__init__.py
Outdated
There was a problem hiding this comment.
This hasn't been formatted properly, make sure to poetry install --with dev and pre-commit install. You can also manually install black and run it from the root of the project with black ..
pyasic/config/fans.py
Outdated
There was a problem hiding this comment.
Maybe wrap in a try: except LookupError: here instead of using get, would rather return default if it fails to parse any of the keys.
pyasic/config/temperature.py
Outdated
| hot_temp = web_conf["Misc"]["Shutdown Temp"] | ||
| dangerous_temp = web_conf["Misc"]["Shutdown Temp"] | ||
|
|
||
| if web_conf["Fans"]["Fan Mode"].get("Auto") is not None: |
There was a problem hiding this comment.
Not sure about this one, but it may also benefit from a try: except LookupError:.
pyasic/config/temperature.py
Outdated
| target_temp = web_conf["Fans"]["Fan Mode"]["Auto"]["Target Temperature"] | ||
|
|
||
| if web_conf.get("Misc") is not None: | ||
| return cls( |
There was a problem hiding this comment.
I think there might be a better way to write this, if you're using get. These are some of the only values in the config that are allowed to be None, so maybe with using get and checks for lookup error there's some nicer way to write it.
|
I see why try, except works better. I believe it can now catch all situations and always return at worst the default. If this seems ok we can merge this as part 1, I will add the additional changes once your changes are merged. |
Part 1 of 2. Partially closes #80 but currently only handles the get_config side. Want to break this up into two parts + prepare for changes in the pipeline for further configuration options.
Let me know if this tracks with the current methodology, then can continue with the send_config side.