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

Type extension aiopika workaround #292

Closed

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Dec 1, 2024

Based on #289

Copy link

codecov bot commented Dec 1, 2024

Codecov Report

Attention: Patch coverage is 86.48649% with 15 lines in your changes missing coverage. Please review.

Project coverage is 90.78%. Comparing base (14b7c1a) to head (ef7b10b).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/plumpy/event_helper.py 0.00% 2 Missing ⚠️
src/plumpy/ports.py 85.72% 2 Missing ⚠️
src/plumpy/process_states.py 75.00% 2 Missing ⚠️
src/plumpy/processes.py 83.34% 2 Missing ⚠️
src/plumpy/communications.py 75.00% 1 Missing ⚠️
src/plumpy/events.py 75.00% 1 Missing ⚠️
src/plumpy/persistence.py 85.72% 1 Missing ⚠️
src/plumpy/process_comms.py 66.67% 1 Missing ⚠️
src/plumpy/process_listener.py 0.00% 1 Missing ⚠️
src/plumpy/process_spec.py 50.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
+ Coverage   90.77%   90.78%   +0.01%     
==========================================
  Files          22       22              
  Lines        2990     2980      -10     
==========================================
- Hits         2714     2705       -9     
+ Misses        276      275       -1     

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

@unkcpz unkcpz mentioned this pull request Dec 1, 2024
1 task
@sphuber
Copy link
Collaborator

sphuber commented Dec 1, 2024

What is the scope of this PR @unkcpz You referenced it in #289 saying that this should just fix the typing_extensions import error. But there seem to be a lot of changes here.

@unkcpz
Copy link
Member Author

unkcpz commented Dec 1, 2024

After #289 there is only the last commit will fix typing_extensions error. That one should go first.

@sphuber
Copy link
Collaborator

sphuber commented Dec 1, 2024

After #289 there is only the last commit will fix typing_extensions error. That one should go first.

I don't understand. Do you mean in this PR you just mean to merge ef7b10b ? If so, could you please update the branch to just include that commit?

@unkcpz
Copy link
Member Author

unkcpz commented Dec 1, 2024

Do you mean in this PR you just mean to merge ef7b10b ?

Yes.

If so, could you please update the branch to just include that commit?

But the PR was from the branch of my fork.
Sorry if it is confusing.

@sphuber
Copy link
Collaborator

sphuber commented Dec 1, 2024

Yeah, but now this PR would change over a 1000 lines. Surely that is not the intention?

@unkcpz
Copy link
Member Author

unkcpz commented Dec 1, 2024

Yes, I was waiting for the merge of #289 :) I should put "blocked" tag for this one.

@unkcpz unkcpz added the pr/blocked PR is blocked by another PR that should be merged first label Dec 1, 2024
@sphuber
Copy link
Collaborator

sphuber commented Dec 1, 2024

But there the CI is failing because of the fix you put here? Why not just put just that fix in this PR, we merge that, and then you update the other PR and tests will pass. Since the fix doesn't touch any of the other changes, there should be no conflicts whatsoever. In fact, you would just do on the branch of this PR

git reset --hard origin/master
git cherry-pick ef7b10b3b4226056b59be4c4b9da8e8d7b399fc5

Now you just have the commit with the fix

@unkcpz unkcpz closed this Dec 1, 2024
@unkcpz unkcpz force-pushed the type-extension-aiopika-workaround branch from ef7b10b to ba77abf Compare December 1, 2024 16:37
@unkcpz
Copy link
Member Author

unkcpz commented Dec 1, 2024

I see. Not sure what I did I tried with cherry-pick but it closes the PR. I'll open a new one with the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/blocked PR is blocked by another PR that should be merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants