Skip to content

Inheritance not working with non-abstract base methods #756

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

Merged
merged 4 commits into from
Oct 29, 2018
Merged

Inheritance not working with non-abstract base methods #756

merged 4 commits into from
Oct 29, 2018

Conversation

@den-run-ai
Copy link
Contributor

@smourier what should be the logic if the types are not equal and they don't derive from one another?

Not sure what is going on with both CI, first time this happens! I just tried to restart some builds, but looks like the problem is on the github side.

@smourier
Copy link
Contributor Author

I choose to modify the existing Comparer to minimize regression risks since I don't know this project much, and I don't know python much either :-).

Like I said, the whole Bind code that chooses what method should be taken is quite fragile (also I don't really understand the funky GetPrecedence algorithm?) and cannot support all different possibilities, it's not only a matter of sorting the methods.

If you want to support all the different types of methods that the .NET framework supports (including generics), you probably need to add some options that the caller could use on the python side, IMHO

PS: yes, github seems in trouble today!

@den-run-ai
Copy link
Contributor

github is still recovering from network/database failure:

https://blog.github.com/2018-10-21-october21-incident-report/

@den-run-ai
Copy link
Contributor

All existing tests are passing, but this new code needs a unit test.

@den-run-ai
Copy link
Contributor

@AlexCatarino this looks like a fix for your issue #552?

@AlexCatarino
Copy link
Contributor

Yes, @denfromufa , thank you @smourier!

@filmor
Copy link
Member

filmor commented Oct 23, 2018

@smourier Thanks a lot for this patch, from my point of view this is good to go, but please, if possible

  1. Add yourself to the AUTHORS file
  2. Add a test
  3. Add the fix to the Changelog

If you don't have the time for these, say the word and I'll complete the PR s.t. it can go into the next release :)

@smourier
Copy link
Contributor Author

smourier commented Oct 23, 2018

Well, originally, I was just answering a question on stackoverflow, so I let you guys the complete what needs to be :)

@bminixhofer
Copy link

bminixhofer commented Oct 24, 2018

Although this is not yet released, I will close the question on StackOverflow because it does not need an answer anymore. Thanks for fixing this @smourier!

@codecov
Copy link

codecov bot commented Oct 24, 2018

Codecov Report

Merging #756 into master will decrease coverage by 0.02%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #756      +/-   ##
==========================================
- Coverage   77.08%   77.05%   -0.03%     
==========================================
  Files          63       63              
  Lines        5729     5736       +7     
  Branches      904      907       +3     
==========================================
+ Hits         4416     4420       +4     
- Misses       1016     1017       +1     
- Partials      297      299       +2
Flag Coverage Δ
#setup_linux 69.42% <ø> (ø) ⬆️
#setup_windows 76.25% <57.14%> (-0.03%) ⬇️
Impacted Files Coverage Δ
src/runtime/methodbinder.cs 89.8% <57.14%> (-0.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5d299e...2fff248. Read the comment docs.

@filmor filmor requested a review from den-run-ai October 29, 2018 12:06
@filmor filmor added the next label Oct 29, 2018
@filmor filmor added this to the 2.4.0 milestone Oct 29, 2018
@den-run-ai den-run-ai merged commit 2857071 into pythonnet:master Oct 29, 2018
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.

5 participants