Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
slide wants to merge3 commits intopythonnet:masterfromslide:int_parameter_method_resolution
Closed

Fix #1523#1524

slide wants to merge3 commits intopythonnet:masterfromslide:int_parameter_method_resolution

Conversation

slide
Copy link
Contributor

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.

  • Make sure to include one or more tests for your change
  • Add yourself toAUTHORS
  • Updated theCHANGELOG

Adds a preferred type parameter to GetTypeByAlias and thenthe preferred type gets passed in in methodbinder'sTryComputeClrArgumentType
@lostmsu
Copy link
Member

This fails some tests.
Have you explored what happens when you removeneedsResolution altogether?

@slide
Copy link
ContributorAuthor

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.

@lostmsu
Copy link
Member

@slide master fails pretty rarely, and only in 1 configuration, probably due to heap corruption. The failure here is directly related to the change.

@slide
Copy link
ContributorAuthor

Ok, I'll definitely take a look. I need to stop using my phone for looking at github

@slide
Copy link
ContributorAuthor

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)
@slide
Copy link
ContributorAuthor

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();
Copy link
Member

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?

@slide
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

slide commentedAug 25, 2021
edited
Loading

Closed in favor of#1531

@slideslide closed thisAug 25, 2021
@slideslide deleted the int_parameter_method_resolution branchAugust 25, 2021 18:47
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@lostmsulostmsulostmsu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@slide@lostmsu

[8]ページ先頭

©2009-2025 Movatter.jp