- Notifications
You must be signed in to change notification settings - Fork749
Fix #1523#1524
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix #1523#1524
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This fails some tests. |
It looks like master is also failing the tests? I may be wrong since I only looked on my phone. I have not tried removing needsResolution, I can try that. |
@slide master fails pretty rarely, and only in 1 configuration, probably due to heap corruption. The failure here is directly related to the change. |
Ok, I'll definitely take a look. I need to stop using my phone for looking at github |
Ok, I can see the failures much easier on my computer. It looks like a different overload is being chosen for the <= operator. I'll track down the issue and see why it is happening. |
This sorts matching methods so that non PyObject overloadsare looked at first. The other option for this would be to changethe test such that it determines the type before assuming that itis a tuple (See public static bool operator <=(OperableObject a, PyObject b)int TestOperator.cs)
I don't know if the solution I proposed is a good one for all circumstances. Another way of fixing the tests would be to change the overload that takes a PyObject to not assume that it is a tuple. |
// this when there is more than one arg matched method | ||
if (argMatchedMethods.Count > 1) | ||
{ | ||
argMatchedMethods = argMatchedMethods.OrderBy(x => x.HasPyObjectParameter).ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is the scenario? Why is this change needed?
In the case of the test, there are two overloads that could match, one that has (OperableObject, int) and one that has (OperableObject, PyObject) as parameters. The one with PyObject is "matched" first and all the arguments can be coerced to the required parameters, so this method is chosen as the matching method to execute. That overload assumes that the PyObject argument is a tuple rather than a simple integer. This change sorts the matching methods so that ones with PyObject parameters would be checked last. As I said in my last comment, I don't know if this is a good solution, or if I should just change the test. It seems like users would want more specific methods matched first before PyObject parameter ones, but again, I am not sure. |
slide commentedAug 25, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Closed in favor of#1531 |
What does this implement/fix? Explain your changes.
Adds a preferred type parameter to GetTypeByAlias and then the preferred type gets passed in in methodbinder's TryComputeClrArgumentType. I am not sure if this is the best fix for the issue, I know that the test fails before the fix and passes after.
Does this close any currently open issues?
#1523
Checklist
Check all those that are applicable and complete.
AUTHORS
CHANGELOG