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.

Sign up for GitHub

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

Jump to bottom

Userutil #244

Merged
merged 8 commits into from
Feb 27, 2024
Merged

Userutil #244

merged 8 commits into from
Feb 27, 2024

Conversation

SpheMakh
Copy link
Collaborator

Fixes #222

For an example, see this

@SpheMakh SpheMakh self-assigned this Feb 21, 2024
o-smirnov
o-smirnov previously approved these changes Feb 23, 2024
View reviewed changes
scabha/schema_utils.py Outdated
outputs: Dict[str, Parameter]
libs: Dict[str, Any]

def paramfile_loader(paramfiles: List[File], sources: List[File] = [], schema_spec=None, use_cache=False) -> Dict:
Copy link
Member

Choose a reason for hiding this comment

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

Suggest Union[File|List[File]], since that's how you treat it in the code below. Just to make the function signature more self-explanatory.

@o-smirnov
Copy link
Member

Happy to merge, the above suggestion is purely cosmetic.

o-smirnov
o-smirnov approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@o-smirnov o-smirnov left a comment

Choose a reason for hiding this comment

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

LGTM

@o-smirnov o-smirnov merged commit de670f7 into master Feb 27, 2024
4 checks passed
@o-smirnov o-smirnov deleted the userutil branch February 27, 2024 14:12
@o-smirnov
Copy link
Member

@SpheMakh ooops that broke things. And I missed it in my review. I blame MK@5 🤣

@o-smirnov
Copy link
Member

File|List[File] should be Union[File, List[File]].

o-smirnov added a commit that referenced this pull request Mar 2, 2024
@o-smirnov
fixes breakage from #244
e172be5
@SpheMakh
Copy link
Collaborator Author

SpheMakh commented Mar 2, 2024

Ahh, the | operator is the same as a Union, so int|float is the same as Union[int|float]. But I jumped the gun because this is only implemented in versions 3.10 and above.

But how did the packaging github action miss this 🤔

@o-smirnov
Copy link
Member

I'm not sure. Does it run a linter?

@o-smirnov
Copy link
Member

I think it's because none of the current tests import schema_utils. The error is thrown upon import. Anyway, easy fix, please approve #248.

SpheMakh added a commit that referenced this pull request Mar 2, 2024
@SpheMakh
Merge pull request #248 from caracal-pipeline/userutil-fix
7d02af6 
fixes breakage from #244
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@o-smirnov o-smirnov o-smirnov approved these changes

Assignees

@SpheMakh SpheMakh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add module to ease the *scabha* barrier to entry
2 participants
@SpheMakh @o-smirnov