Add more types for function arguments and return values #86
Conversation
syou6162
force-pushed
the
feature/add_more_types
branch
4 times, most recently
from
3ebc5a2
to
04a6ed2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 31 32 +1
Lines 930 959 +29
=========================================
+ Hits 930 959 +29 ☔ View full report in Codecov by Sentry. |
syou6162
force-pushed
the
feature/add_more_types
branch
4 times, most recently
from
dd45050
to
49b2e6b
Compare
| from dbt_artifacts_parser.parsers.manifest.manifest_v10 import ManifestV10 | ||
| from dbt_artifacts_parser.parsers.manifest.manifest_v11 import ManifestV11 | ||
|
|
||
| Manifest = Union[ |
There was a problem hiding this comment.
I think it would be best if dbt_artifacts_parser provided these types :)
There was a problem hiding this comment.
@syou6162 hmm maybe good to make a pull request to dbt_artifacts_parser?!
I'm just concerning that this Union of all versions, that will get burden of dbterd side which will require a change to align with dbt_artifacts_parser. This is also a reason why we relax its version here.
Do you have any ideas to avoid that? Thanks
There was a problem hiding this comment.
@datnguye I think your concern is valid.
OK, I understand. I will try to send a pull request to dbt-artifacts-parser to provide these types. However, it may take some time for the pull request to be reviewed or incorporated into the new version. In my experience, it often takes a long time to be reviewed. By the way, this repository is very helpful for me as a contributor because reviews are done quickly 👍.
When a third party like me is involved in the development, it is a big help to have types in the arguments and return values of functions using mypy. So how about proceeding in this way?
- Type definition using
Unionin dbterd (this pull request) - Send a pull request to dbt-artifacts-parser to add the
Uniontype definition, and it will be included in the new version. - Replace the types defined in dbterd with the types defined in dbt-artifacts-parser
If you have difficulty accepting this proposal, I will add a new commit to remove the type definitions for Manifest and Catalog for now. However, it is the type definitions for Manifest and Catalog that I originally mainly wanted to add 😭
There was a problem hiding this comment.
@datnguye I have done some investigation and will add a note. It is possible to get the return type of a function via mypy, but it is not a good idea since it is displayed as Any on IDEs like VSCode, and it is not easy to get to the type definition. It seems that this type information can only be obtained at runtime.
from typing import get_type_hints, TypeAlias
from dbt_artifacts_parser.parser import parse_manifest
Manifest: TypeAlias = get_type_hints(parse_manifest).get("return")
tmp: Manifest
There was a problem hiding this comment.
Thanks @syou6162 I think I will be convinced by current impl, but let me take a look further until Tuesday. If we can not see any alternatives, lets merge it right away and tag v1.11.0b2.
The major 1.11.0 should be available by end of next week 👍 🏃
There was a problem hiding this comment.
Just FYI, I have submitted a pull request to dbt-artifacts-parser to provide a type using Union.
syou6162
force-pushed
the
feature/add_more_types
branch
from
49b2e6b
to
51f41f7
Compare
syou6162
changed the title
resolves #79
Description
As far as I can understand from reading the code, I have assigned types to the arguments and return values of the functions.
Checklist