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

Added typing to functions and improved docstrings to be more descriptive. #177

Merged
merged 37 commits into from
Jun 20, 2022
Merged

Added typing to functions and improved docstrings to be more descriptive. #177

merged 37 commits into from
Jun 20, 2022

Conversation

BeatsuDev
Copy link
Contributor

Added typing to all the functions and parameters in __init__.py. Also added Sphinx-styled documentation to explain the parameters of the functions.

@Danielhiversen
Copy link
Owner

Thanks.
Please fix the failing tests

@BeatsuDev
Copy link
Contributor Author

I think it should be good now 👍

@BeatsuDev
Copy link
Contributor Author

BeatsuDev commented May 9, 2022

I read through the code again and made some small changes/fixes (ref: last two commits on this PR). These shouldn't break anything or throw any new warnings or errors.

@Danielhiversen
Copy link
Owner

Great.
Mypy should also be added to test the typing

https://github.com/Danielhiversen/pyTibber/pull/131/files#diff-51335077549f8f95664f409b515ccf2019991e216d97f29b790ddde6c4be4033R46-R48

@BeatsuDev
Copy link
Contributor Author

Didn't know about mypy. Seems like a great tool! I'll look into it, thanks! 😄

@BeatsuDev
Copy link
Contributor Author

BeatsuDev commented May 9, 2022

Notable changes after using mypy:

  • I changed the declaration of variables from self.foo: str = None to self.foo: str = "". This was only changed in the last commit and can be easily reverted if it causes problems.

  • Adding to the previous point, I changed the initial value of self.last_data_timestamp, self.peak_hour_time and self.last_cons_data_timestamp to dt.datetime.min. I checked the first and last properties for usages and since they are only used to check if they're lower than another value, it should be fine.

  • Some places I had to add return None instead of simply return to stop mypy from complaining.

  • Since Tibber.execute can return None, I added an if statement and only set the value of self.info if the return value of .execute is defined. I've added TODO comments for potential error handling in the code.

self.info = data
self._process_price_info(self.info)
else:
# TODO: Error handling?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it's okay to just return None instead of overwriting self.info with a potential None value? Does this break any functionality? There are 2 other places where I've done this (3 in total)

Copy link
Contributor Author

@BeatsuDev BeatsuDev May 15, 2022

Choose a reason for hiding this comment

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

@Danielhiversen ^ The question is really: When a user fetches data, but it fails to get that information because of a request error, should the method return None, overwrite self.info with value None, or should an exception be raised?

Copy link
Owner

Choose a reason for hiding this comment

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

return None is ok for this pr

@Danielhiversen
Copy link
Owner

Have you tested your changes locally?

@BeatsuDev
Copy link
Contributor Author

BeatsuDev commented May 31, 2022

Have you tested your changes locally?

Yep! The only workflow I could not get to run was the isort workflow. I don't think that's a problem with the repo however, but rather on my side...

isort.exceptions.InvalidSettingsPath: isort was told to use the settings_path: C:\Users\Beatsu\Documents\Coding\GitHub\pyTibber\** as the base directory or file that represents the starting point of config file discovery, but it does not exist.

@BeatsuDev
Copy link
Contributor Author

Running isort . however (which should be the equivalent to isort **/*.py?) changes the imports in setup.py (which I haven't touched) and test.py (which I also haven't touched) as well as __init__.py. Shall I apply this change?

@BeatsuDev
Copy link
Contributor Author

I actually double-checked and just realised that the tests are being skipped because pytest-asyncio is not installed... The third test then fails because at assert tibber_connection.name is None, because the name is an empty string instead (""). I'll look into changing all/most the properties into Optional[...] types instead.

tibber/__init__.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@BeatsuDev
Copy link
Contributor Author

BeatsuDev commented Jun 8, 2022

I added a new dependency (pytest-asyncio) so that the tests could be run and not just be skipped (see issue #178). There were some issues with the cache so the setup of the workflow failed. Therefore removed the caching step and used python-setup's built-in caching feature instead. This required a requirements.txt to install from afaik, so I added that. In short, the last changes done in this PR will fix #178.

@BeatsuDev
Copy link
Contributor Author

I could revert these last changes (requirements.txt and pytest-asyncio) and add them in a different PR if you'd like. Shall I do that?

@Danielhiversen
Copy link
Owner

Danielhiversen commented Jun 9, 2022

Fixed by #181

requirements.txt Outdated Show resolved Hide resolved
@BeatsuDev
Copy link
Contributor Author

Not sure why pylint did not use the setup.cfg in this branch, but it worked fine in the master branch... Anyway, I fixed it by shortening the line 👍

@Danielhiversen Danielhiversen merged commit 90ff839 into Danielhiversen:master Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants