Skip to content

Refactoring of MethodBinder.Bind #829

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 6 commits into from
Mar 19, 2019

Conversation

lostmsu
Copy link
Member

@lostmsu lostmsu commented Mar 12, 2019

What does this implement/fix? Explain your changes.

This is a refactoring/cleaning up of MethodBinder.Bind method. No new features were introduced.

Does this close any currently open issues?

This is a preliminary work necessary for #823.

Any other comments?

The refactoring was mostly done using ReSharper with little manual interventions. The semantics should be preserved.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@lostmsu lostmsu changed the title Pr/method binder refactoring Refactoring of MethodBinder.Bind Mar 12, 2019
@lostmsu lostmsu marked this pull request as ready for review March 13, 2019 04:45
@lostmsu
Copy link
Member Author

lostmsu commented Mar 13, 2019

The build failure seems to be simple build timeout. Very unlikely caused by this change.

@filmor
Copy link
Member

filmor commented Mar 13, 2019

I'll have a closer look at this at the weekend. No idea why travis is not getting triggered here, I've poked the one part that timed out though, so this one should go green.

Thanks already :)

@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #829 into master will decrease coverage by 0.74%.
The diff coverage is 94.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #829      +/-   ##
==========================================
- Coverage   76.85%   76.11%   -0.75%     
==========================================
  Files          63       63              
  Lines        5786     5798      +12     
  Branches      943      950       +7     
==========================================
- Hits         4447     4413      -34     
- Misses       1025     1071      +46     
  Partials      314      314
Flag Coverage Δ
#setup_linux ?
#setup_windows 76.11% <94.84%> (+0.04%) ⬆️
Impacted Files Coverage Δ
src/runtime/methodbinder.cs 90.26% <94.84%> (+0.45%) ⬆️
setup.py 69.96% <0%> (-16.26%) ⬇️

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 93a8c83...d866e33. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #829 into master will increase coverage by 0.04%.
The diff coverage is 94.84%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #829      +/-   ##
=========================================
+ Coverage   76.85%   76.9%   +0.04%     
=========================================
  Files          63      63              
  Lines        5786    5798      +12     
  Branches      943     950       +7     
=========================================
+ Hits         4447    4459      +12     
  Misses       1025    1025              
  Partials      314     314
Flag Coverage Δ
#setup_linux 68.55% <ø> (ø) ⬆️
#setup_windows 76.11% <94.84%> (+0.04%) ⬆️
Impacted Files Coverage Δ
src/runtime/methodbinder.cs 90.26% <94.84%> (+0.45%) ⬆️

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 93a8c83...d866e33. Read the comment docs.

@filmor
Copy link
Member

filmor commented Mar 14, 2019

I'll just close and reopen this one to poke Travis...

@filmor filmor closed this Mar 14, 2019
@filmor filmor reopened this Mar 14, 2019
@lostmsu
Copy link
Member Author

lostmsu commented Mar 14, 2019

CI still having internal problems :/

for (var v = pynargs; v < clrnargs; v++)
//CLRObject co = (CLRObject)ManagedType.GetManagedObject(inst);
// InvalidCastException: Unable to cast object of type
// 'Python.Runtime.ClassObject' to type 'Python.Runtime.CLRObject'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment confuses me more than it helps ;). It's clear after reading the next one, but here not so much.

{
// TODO: is this a bug? Should this happen even if the conversion fails?
// GetSlice() creates a new reference but GetItem()
// returns only a borrow reference.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indeed looks like a bug to me, the code has to also run before return null.

@filmor
Copy link
Member

filmor commented Mar 15, 2019

I think both comments are for things that you didn't change, just wanted to leave them around.

@filmor filmor merged commit 5ee234a into pythonnet:master Mar 19, 2019
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.

2 participants