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

Implement dedicated causes object #93

Closed
wants to merge 11 commits into from

Conversation

notatallshaw
Copy link
Collaborator

@notatallshaw notatallshaw commented Oct 30, 2021

This is proof of concept to address issues raised in #92 (comment)

Derived information can then be calculated and cached as needed. In particular the downstream provider can run identifier in backtrack_causes.names where names is a property that calculates the underlying information lazily and only has to be re-calculated if the underlying causes are changed.

I'm trying to see if this approach would have consensuses, several things in the code would need fixing before merging.

@notatallshaw
Copy link
Collaborator Author

notatallshaw commented Oct 30, 2021

A nice thing about this approach is it allows the downstream library to be decoupled from the internal structure of the causes. Maybe helpful?

@notatallshaw notatallshaw mentioned this pull request Oct 31, 2021
Can't use property setter to update causes
because it conflicts with State being a named tuple
@uranusjr
Copy link
Member

I think I like this approach the best, but we need to go a bit further down the decoupling path. Taking things like pypa/pip#10478 into account, perhaps we should take a similar approach to Requirement and Candidate and add an interface in Provider custom Cause object logic? (We can provide a default implementation if that’s a good idea.)

@notatallshaw
Copy link
Collaborator Author

That makes sense to me, I will take a look at how Requirement and Candidate are implemented and mirror that approach.

@hswong3i
Copy link

So #97 and 1.0.0 should now depends on this PR?

@uranusjr
Copy link
Member

uranusjr commented Nov 18, 2021

Yes.

Technically not this PR specifically, but the implementation of a new provider method that allows creating a custom conflict cause object that the resolver will modify and pass back to other provider hooks. That may happen in this PR, another PR, or a combination of several.

@hswong3i
Copy link

Seems a huge workload for Cause, which may need more than a month for implement it precisely… Maybe this breaking change could go into 2.0.0? Or just implement the API interface changes now for 1.0.0 without indeed and perfect implementation, yet?

@jbylund
Copy link
Contributor

jbylund commented Nov 18, 2021

Technically not this PR specifically, but the implementation of a new provider method that allows creating a custom conflict cause object that the resolver will modify and pass back to other provider hooks. That may happen in this PR, another PR, or a combination of several.

Isn't that what #96 is? Using that approach a provider could pass make_causes_obj_from_raw_causes if it wanted to (though my preference would be for just set of strings).

@uranusjr
Copy link
Member

Yes except I want it to be a provider method and create the entire cause object, not modify it.

@notatallshaw
Copy link
Collaborator Author

Not had any time to work on this this last month. However I expect to have some free time during the holidays. I'm probably going to create a new PR and close this one, implementing it like Requirement and Candidate the way @uranusjr suggests.

I don't feel any ownership over doing the work 😉, so if anyone else wants to take the approach and implement it themselves feel free to, otherwise I will come back with an update later this month.

@notatallshaw
Copy link
Collaborator Author

I think I like this approach the best, but we need to go a bit further down the decoupling path. Taking things like pypa/pip#10478 into account, perhaps we should take a similar approach to Requirement and Candidate and add an interface in Provider custom Cause object logic? (We can provide a default implementation if that’s a good idea.)

I've addressed these comments in a new PR: #99 and an accompanying pip PR: pypa/pip#10732

@notatallshaw notatallshaw deleted the cause_object branch November 13, 2022 18:57
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.

4 participants