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

feat: Try to inject parameter value if default value is None #110

Merged
merged 6 commits into from
Apr 22, 2024

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented Apr 22, 2024

Currently, in-n-out do not allow injecting parameters for function with default value. This does not allow creating an action that has optional injection but will not crash on injection failure.

A workaround for this may be modifying the signature of the implementation to remove the default value information from the signature. But it is really hacky.

Alternative to this, PR may define a special generic type to inform that injection should be performed even if a default value is provided.

Copy link

codspeed-hq bot commented Apr 22, 2024

CodSpeed Performance Report

Merging #110 will not alter performance

Comparing Czaki:inject_on_none (da6e239) with main (190cdb9)

Summary

✅ 5 untouched benchmarks

@Czaki
Copy link
Contributor Author

Czaki commented Apr 22, 2024

it looks like codecov@v3 is broken. Maybe we should add combine steep for codecov?

@tlambert03 tlambert03 merged commit 8f8ec35 into pyapp-kit:main Apr 22, 2024
8 of 22 checks passed
@tlambert03
Copy link
Member

thanks @Czaki, I added a test.
The issue with codecov is annoying. I want to use codecov v4, but I also want to keep using reusable workflows, and I'm struggling to pass the token to the reusable workflow when the action is triggered by a PR from a fork. How are you dealing with that? I see a similar setup in your workflows in napari, where you're just passing:

    uses: ./.github/workflows/reusable_coverage_upload.yml
    secrets:
      codecov_token: ${{ secrets.CODECOV_TOKEN }}

but for some reason it's working there but not here. (note that I currently use secrets: inherit, but I don't think it matters... I also tried the explicit passing of codecov-token). Anyway, I'll figure that out in a later PR. Thanks for this good fix

@tlambert03 tlambert03 added the enhancement New feature or request label Apr 22, 2024
@Czaki
Copy link
Contributor Author

Czaki commented Apr 22, 2024

We do not do this. Codecov fixed their action and version 4 allow tokenless upload from a fork of public repository.

However time to time we hit api response problems. Because of this we upload coverage in separate steep.

@Czaki Czaki deleted the inject_on_none branch April 22, 2024 20:20
@tlambert03
Copy link
Member

Codecov fixed their action and version 4 allow tokenless upload from a fork of public repository.

oh! well that's exciting. somehow I missed this important detail 😂 that's great.

@Czaki
Copy link
Contributor Author

Czaki commented Apr 22, 2024

oh! well that's exciting. somehow I missed this important detail 😂 that's great.

It was fixed a few months after v4 release

@tlambert03
Copy link
Member

thanks. yeah, it's all working fine now. I just removed the codecov v3 fallback logic from the reusable workflow and it just does the right thing for PRs from forks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants