feat: including typehints; tox to manage packages; mypy to static check; remove unnecessary requirements; change circle ci to use tox; run black and isort to format #118
Conversation
…ck; remove unnecessary requirements; change circle ci to use tox; run black and isort to format
pedroimpulcetto
changed the title
There was a problem hiding this comment.
Hey @pedroimpulcetto, thank you for your contribution. I'm excited to see our client getting improvements!
I've left a few comments on things I noticed right away, however I'd really appreciate if we did this as a series of smaller, more focused, separate PRs. I'd be happy to help review and ship those.
| @@ -15,10 +15,13 @@ defaults: &defaults | |||
| - checkout | |||
| - run: | |||
| name: Install dependencies | |||
| command: pip install -r requirements.txt | |||
| command: pip install tox | |||
There was a problem hiding this comment.
We're trying to move away from using CircleCI and towards github actions. Would you mind holding off any changes to CircleCI config?
| """Construct and return a requests.Request object based on | ||
| provided parameters. | ||
| """ | ||
| if api_key: | ||
| auth = (api_key, '') | ||
| auth = (api_key, "") |
There was a problem hiding this comment.
I'd prefer shipping changes to formatting separately from functional ones - putting them together makes actual changes really hard to review.
There was a problem hiding this comment.
I will make a different PR for isort and black after having specific requirements for dev/test dependencies
| @@ -0,0 +1,3 @@ | |||
| [mypy] | |||
There was a problem hiding this comment.
We prefer tooling configuration to be placed in pyproject.toml instead of tool-specific files.
There was a problem hiding this comment.
as this project is not using pyproject.toml I'm going to include in setup.cfg
| @@ -1,4 +1 @@ | |||
| pytest==7.4.0 | |||
There was a problem hiding this comment.
We still need to pin specific dependency versions. Splitting them into a separate file is a good idea though (example in another repo).
There was a problem hiding this comment.
do you prefer to have only one requirements-dev.txt for all development dependencies or have different requirements-lint.txt, requirements-test.text, etc?
There was a problem hiding this comment.
I just pushed this new PR #119 creating a new requirements-dev.txt file
| commands = | ||
| pytest | ||
|
|
||
| [testenv:format] |
There was a problem hiding this comment.
I don't think we should be using tox as a framework for running other tools besides doing actual testing.
I really appreciate your time to take a look at it @tsx, thank you! |
Hey there, I'm trying to keep this repository alive by implementing Mypy as a static type checker and making some small improvements, not changing the repository core.
I hope it looks good 🙏 but feel free to suggest or decline any changes.
🛠️ Adding Mypy for Better Type Checking
A development dependency,
Mypy, was added. It checks for type-related errors before you run the code, making it less prone to runtime errors and smoothing the development process.📦 Dependency Changes in
requirements.txtThe dependencies
pytestandresponseshave been removed. These dependencies aren't necessary for thecloseio_apipackage as they are dev-dependencies, so they were added intox.inifor testing purposes only.🔄 Updates in
.circleci/config.ymlWe are now using
pip install toxto run tests and Mypy. A new step, 'Mypy', has been introduced, which runs Mypy on the codebase, ensuring type safety.💡 Type Hints in
closeio_apiSeveral improvements have been made to the
closeio_api/__init__.pyfile. The main changes include adding type hints to variables and function parameters. This enhancement helps catch type errors at an early stage, making the code safer and more maintainable.