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

Allow Closing to detect dependent resources passed as kwargs too #636 #653

Closed
wants to merge 5 commits into from

Conversation

federinik
Copy link

A small addiction to the work of @StummeJ about issue #633 which allows to benefit of his improvement even for dependencies passed as kwargs (such as my case)

@antongmbl
Copy link

Hey, any plans to review and merge this one ?

@federinik
Copy link
Author

Friendly reminder that this 2-lines PR (rest is just testing) has been open and unmerged for 3 months now

@jazzthief
Copy link

Hey, @federinik! I have recently encountered the same issue as you, plus some more (#702 has some details on one of the problems I faced), and started making changes to _locate_dependent_closing_args() before I found this PR. Since we work on the same bit of functionality, do you think we could merge our changes together into a single PR? I think it would be wise in terms of saving the reviewers' time.

@federinik
Copy link
Author

federinik commented May 18, 2023

Hi @jazzthief, sure thing, feel free to include what I wrote in a common PR/branch or, if you prefer, I could give you access to my fork of the project. Anyway, I'm not really optimistic about receiving soon updates from the reviewers, this PR has been open a while now, unluckily.

Let me know how I can help!

@jazzthief
Copy link

@federinik Thank you! I would really like to somehow preserve this PR and give credit to the fact that you first found this problem. So I think I am going to open a PR to combine all the changes and ask you to change your PR's target branch to mine. This way, I can merge your PR and have control over the description of all the changes I am going to make. Does that work for you?

@federinik
Copy link
Author

Hi @jazzthief, sound good to me. Let me know when everything is set up and I'll open a PR to your branch. Thank you!

@jazzthief
Copy link

@federinik I've created the PR: #711. The branch I work on is GodelTech:702-nested-resource-resolution.

@federinik federinik force-pushed the master branch 3 times, most recently from 7195cbd to f3d7e68 Compare May 23, 2023 10:38
@federinik
Copy link
Author

Changes integrated in PR #711, I'm closing this. Thank you all!

@federinik federinik closed this May 23, 2023
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.

None yet

3 participants