Skip to content

Conversation

@GlassOfWhiskey
Copy link
Collaborator

This commit adds more TypedDict definitions for File, Dirent, and Directory objects. In addition, it adds the exitCode entry to the CWLRuntimeParameterContext, which was missing.

@GlassOfWhiskey GlassOfWhiskey force-pushed the more-on-types branch 3 times, most recently from e5420a6 to 8ae3b07 Compare December 13, 2025 08:05
@GlassOfWhiskey GlassOfWhiskey requested a review from mr-c December 13, 2025 08:29
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I think there are some Seven Bridges workflows that decorate input and output Files and Directories with additional fields and reference those fields from CWL Parameter Expressions.

See common-workflow-language/common-workflow-language#710 for some references to this.

So I don't think we can discard or throw an error on references to additional fields. An engine could optimize for the standard fields and use a slower path for extension fields, if that helps.

@GlassOfWhiskey
Copy link
Collaborator Author

Is that allowed by the standard? Shouldn't it be a ns:MyFile class object that extends File?

@GlassOfWhiskey
Copy link
Collaborator Author

@mr-c btw I think I fixed it. I also foud a slightly better way to check keys through TypeGuard functions. However, I am quite surprised there is no way to automatically access the Literal type of TypedDict keys from the TypedDict definition. I can obviously to TypedDict.__annotations__.keys() but this is not typed as Literal so it does not work.

@mr-c
Copy link
Member

mr-c commented Dec 13, 2025

Is that allowed by the standard? Shouldn't it be a ns:MyFile class object that extends File?

Extra namespaced fields are allowed

GlassOfWhiskey added a commit to common-workflow-language/cwltool that referenced this pull request Dec 13, 2025
This commit checks that the new `TypedDict` type aliases specified in
cwl-utils with PR common-workflow-language/cwl-utils#393 do not break
anything in the cwltool type system.
@GlassOfWhiskey GlassOfWhiskey force-pushed the more-on-types branch 2 times, most recently from 11526da to 7c8a166 Compare December 13, 2025 11:56
@codecov
Copy link

codecov bot commented Dec 13, 2025

Codecov Report

❌ Patch coverage is 86.11111% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.69%. Comparing base (b9de378) to head (af8b854).

Files with missing lines Patch % Lines
cwl_utils/sandboxjs.py 66.66% 1 Missing and 1 partial ⚠️
cwl_utils/types.py 92.00% 2 Missing ⚠️
cwl_utils/expression.py 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
+ Coverage   34.66%   34.69%   +0.02%     
==========================================
  Files          30       30              
  Lines       35412    35437      +25     
  Branches     9538     9541       +3     
==========================================
+ Hits        12275    12294      +19     
- Misses      20257    20261       +4     
- Partials     2880     2882       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GlassOfWhiskey GlassOfWhiskey force-pushed the more-on-types branch 4 times, most recently from fa58afd to 985b93e Compare December 14, 2025 16:31
This commit adds more `TypedDict` definitions for `File`, `Dirent`, and
`Directory` objects. In addition, it adds the `exitCode` entry to the
`CWLRuntimeParameterContext`, which was missing.
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