-
Notifications
You must be signed in to change notification settings - Fork 25
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
[3/3] Improve Signature and method table printing #110
Conversation
Ah, that looks beautiful! Love the idea. :) Let me know when you'd like me to review the PR or parts of it. |
I would love some insight on how to compute the distance between (invalid) signatures! |
Ah, let me reply here instead of in the other issue. :) Hmmm, I'm not exactly sure how a solution should look like, but maybe something simple like this could work: def signature_distance(candidate: Signature, reference: Signature):
distance = 0
for t1, t2 in zip(candidate.types, reference.types):
if not issubclass(t1, t2):
distance += 1
return distance This isn't yet quite right, since it doesn't appropriately consider the number of arguments ( Do you have any thoughts or ideas? The above is just the first thing that comes to mind. We could likely do something better. One approach would be to proceed with something simple like this and later improve the distance function. |
output.mp4Ok, more updates... What do you think about this? I think the main thing I would like to do is to replace the values printed in the resolution error with the types, at least for those that are not parametric... but that's not easily done... Because it's hard to understand if the values printed there match or are related to the types below. maybe not hard, but not straightforward |
I absolutely love it!! That looks so good. This is awesome work! :)
This would be really nice, but seems like it would require a |
dfc4b29
to
7b724ce
Compare
@wesselb Ok, I cleaned up the PR. This is now based on top of #112 . It should be possible to implement this logic without the changes of #112 , so if you don't like those changes I could try to remove them from the PR, but in my opinion they modularise a bit the internals of plum (and It made it simpler for me to understand what I was working with.) The major missing things from this PR is now:
I would frankly go for rich, but what do you think? |
Pull Request Test Coverage Report for Build 7596314753Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
d8c2c0e
to
976e08e
Compare
@wesselb I don't want to push, but in case you like/accept the approach and the few internal refactors, I can work my way to add tests and increase coverage again. (I don't want to do the work if you don't like the structure as it would be wasted effort). |
@PhilipVinc sorry for the delay! Yes, I absolutely love this!! It would be awesome to merge this. :) I think the changes in #112 are sensible too. I'll review those first, and then we can merge this. With regards to I'm slightly leaning towards just adding |
a6efbe0
to
27d00d5
Compare
52bbdcd
to
5353c13
Compare
@wesselb The PR is done and the tests have been updated, so I think it is time for a review. Sorry for taking all your time! As you agreed to include Most functionality necessary for converting to displayable objects are in Other minor changes are...
Overall... this logic is ugly as hell, but I don't think it's possible to simplify those repr methods much.. |
use methodlist
omport order
keep str around for more compact repr
a method table a
@PhilipVinc I might add a few more tests, and then I think this is ready to be merged! Are you happy to merge it after a few more tests? |
I think that your last change of method.py , in MethodList, should render the method itself, not the mismatch... |
@PhilipVinc How does this look to you? Ready to merge? |
@PhilipVinc Actually, you've indicated multiple times that you're ready to have this merged, so I'm gonna go ahead to merge this and tag a new release. :) |
very very WIP.
I would also like to experiment with rich to print more coloured, more readable stuff, but this is already more readable...
It refactors a bit the internals. Need to find a better way to do it..