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

refactor: add checks for input types in Task #64

Merged
merged 7 commits into from
Apr 8, 2024

Conversation

gcroci2
Copy link
Contributor

@gcroci2 gcroci2 commented Mar 28, 2024

  • add checks for time vars
  • add checks for all remaining input parameters, for all public methods
  • fix tests
  • add tests for checks
  • ignore ANN401 in tests folder

@gcroci2 gcroci2 marked this pull request as ready for review March 28, 2024 12:52
Copy link
Contributor Author

gcroci2 commented Mar 28, 2024

@gcroci2 gcroci2 changed the title add checks for time vars refactor: add checks for input types in Task Mar 28, 2024
@gcroci2 gcroci2 linked an issue Mar 28, 2024 that may be closed by this pull request
@gcroci2 gcroci2 requested a review from DaniBodor March 29, 2024 13:51
@gcroci2 gcroci2 self-assigned this Mar 29, 2024
@gcroci2 gcroci2 changed the title refactor: add checks for input types in Task add checks for time vars Apr 4, 2024
@gcroci2 gcroci2 changed the title add checks for time vars refactor: add checks for input types in Task Apr 4, 2024
Copy link
Collaborator

@DaniBodor DaniBodor left a comment

Choose a reason for hiding this comment

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

Wouldn't it be easier to use an external package for enforcing type hints for most of these? Either pydantic or something like this (looks less stable, but was easier to find from documentation how to use do it) can do that.

If we stick to leaving it here, I would suggest to make it a bit more modular as per this comment

annubes/task.py Show resolved Hide resolved
annubes/task.py Outdated Show resolved Hide resolved
annubes/task.py Show resolved Hide resolved
annubes/task.py Show resolved Hide resolved
@@ -143,6 +143,7 @@ isort.known-first-party = ["annubes"]
"S101", # Use of `assert` detected
"ANN201", # Missing return type
"D103", # Missing function docstring
"SLF001", # private member access
"SLF001", # Private member access
"ANN401", # Function arguments annotated with too generic `Any` type
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a huge fan of this. I get that it's not pretty to turn it off many times over on individual functions, but I do think it's a good practice to avoid using Any in most cases.

Alternatively, at the top of the test file in question, you can state AnyType = Any #noqa: ANN401, RUF100 as a type alias and then use AnyType wherever you don't want the linter to check without having to noqa each instance.
(the RUF100 ignoring isn't strictly necessary right now, but that is due to a bug in RUF, which I assume will be fixed at some point and then we won't have to fix it post-hoc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the tests, you do want to test Any types. In the code-base I agree, that wouldn't be good. Very likely, we will need to state the exception at the beginning of each test file, so in my opinion is better to just set it in the toml directly on all tests files. This won't disable the error in the code-base, of course.

@gcroci2
Copy link
Contributor Author

gcroci2 commented Apr 8, 2024

Wouldn't it be easier to use an external package for enforcing type hints for most of these? Either pydantic or something like this (looks less stable, but was easier to find from documentation how to use do it) can do that.

I'd go for pydantic, since the other one does not seem very reliable, especially for the future. Pydantic seems to have a dataclasses module, that may do the job.

If we stick to leaving it here, I would suggest to make it a bit more modular as per this comment

As commented above, even if I agree that the checks can be definitely modularised more, it is not so trivial and it requires a bit of more careful edits.

What if I open an issue for evaluating pydantic, and if that doesn't work then we open a PR for making what we have now more modular?

@gcroci2 gcroci2 requested a review from DaniBodor April 8, 2024 13:28
@DaniBodor
Copy link
Collaborator

Wouldn't it be easier to use an external package for enforcing type hints for most of these? Either pydantic or something like this (looks less stable, but was easier to find from documentation how to use do it) can do that.

I'd go for pydantic, since the other one does not seem very reliable, especially for the future. Pydantic seems to have a dataclasses module, that may do the job.

If we stick to leaving it here, I would suggest to make it a bit more modular as per this comment

As commented above, even if I agree that the checks can be definitely modularised more, it is not so trivial and it requires a bit of more careful edits.

What if I open an issue for evaluating pydantic, and if that doesn't work then we open a PR for making what we have now more modular?

Great idea!

Base automatically changed from 52_add_range_func_gcroci2_graphite to main April 8, 2024 14:58
Copy link

sonarcloud bot commented Apr 8, 2024

@gcroci2 gcroci2 mentioned this pull request Apr 8, 2024
@gcroci2 gcroci2 merged commit a89e9f5 into main Apr 8, 2024
8 checks passed
@gcroci2 gcroci2 deleted the 55_add_checks_inputs_gcroci2 branch April 8, 2024 15:06
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.

Add checks for input types in Task
2 participants