hera icon indicating copy to clipboard operation
hera copied to clipboard

Refactor code for annotated type handling

Open jeongukjae opened this issue 1 year ago • 3 comments

Pull Request Checklist

  • [x] ~~Fixes #~~ this PR follows up on #1160
  • [x] Tests added
  • [x] Documentation/examples added : this is refactoring. no need
  • [x] Good commit messages and/or PR title

Description of PR

Currently, we are checking annotated, parameter/artifact, or other types in different ways. In this PR, I added hera._utils.type_util module and unified them into one place. Followings can be expected:

  • If there's another metadata along with Parameter or Artifact, they should be ignored. (e.g. Annotated[type, metadata1, metadata2, Parameter(...)], this can be happened when Annotateds are nested)
  • Optional parameter handling should be more precise which is added in #1160.

Suggestions on function names/module names are welcome.

jeongukjae avatar Aug 21 '24 16:08 jeongukjae

Codecov Report

Attention: Patch coverage is 88.14433% with 23 lines in your changes missing coverage. Please review.

Project coverage is 81.9%. Comparing base (d553546) to head (80425c1). Report is 194 commits behind head on main.

Files with missing lines Patch % Lines
src/hera/workflows/io/_io_mixins.py 76.5% 3 Missing and 8 partials :warning:
src/hera/workflows/cron_workflow.py 66.6% 2 Missing and 3 partials :warning:
src/hera/workflows/_meta_mixins.py 63.6% 2 Missing and 2 partials :warning:
src/hera/shared/_type_util.py 96.2% 0 Missing and 2 partials :warning:
src/hera/workflows/script.py 95.6% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1168     +/-   ##
=======================================
+ Coverage   81.7%   81.9%   +0.2%     
=======================================
  Files         54      61      +7     
  Lines       4208    4747    +539     
  Branches     889    1013    +124     
=======================================
+ Hits        3439    3889    +450     
- Misses       574     625     +51     
- Partials     195     233     +38     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 21 '24 19:08 codecov[bot]

Checked all review comments, feel free to check this PR again :)

jeongukjae avatar Aug 27 '24 15:08 jeongukjae

@alicederyn can you also review this please.

sambhav avatar Aug 27 '24 21:08 sambhav

Thank you all for great reviews! I applied it @elliotgunton @alicederyn

jeongukjae avatar Aug 29 '24 14:08 jeongukjae

I updated to guard multiple type annotations: Last 5 commits This will raise an error with multiple annotations for input/output annotations, and for pydantic fields as well. Thank you again @elliotgunton !

jeongukjae avatar Sep 02 '24 11:09 jeongukjae