Show instance type in constructor signatures #2143
Conversation
Constructor calls now display the instance type instead of None in both signature help and hover. Direct __init__ calls still show -> None. Fixes facebook#1805
|
Hi @sgavriil01! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
| Param::Pos(name, self_type, _) | Param::PosOnly(Some(name), self_type, _), | ||
| ) = params_list.items().first() | ||
| { | ||
| if name.as_str() == "self" || name.as_str() == "cls" { |
There was a problem hiding this comment.
I think the should_override should be inititalized to false and set to true if all the conditions succeed, would reduce the size of these conditionals
| contents | ||
| .get(start..end) | ||
| .map(|callee_text| { | ||
| if callee_text.ends_with(".__init__") { |
There was a problem hiding this comment.
this logic appears duplicated w/ the hover code - maybe we can add a util function to share it?
I also don't really like that we're doing string slicing here - is there a way to do it by detecting the CallTarget? If it's a call to a class name it would show up as CallTarget::Class, directly calling __init__ would not
pyrefly/pyrefly/lib/alt/call.rs
Line 85 in 8052ae3
| "#; | ||
| let report = get_batched_lsp_operations_report_allow_error(&[("main", code)], get_test_report); | ||
| assert!( | ||
| report.contains("-> Person"), |
There was a problem hiding this comment.
can we assert on the whole string in these tests
There was a problem hiding this comment.
I'm curious if we should do def Person(...) -> Person here, it might give it better highlighting properties
|
cc @rchen152 for second review since she wrote the original task |
yangdanny97
left a comment
There was a problem hiding this comment.
Back to you with some comments, thanks
Fixes #1805
Shows the class instance type instead of
-> Nonefor constructor calls in both signature help and hover.Changes
Person()now show-> Personinstead of-> None__init__calls still show-> NoneBox[str])Tests