davidhalter / jedi

Awesome autocompletion, static analysis and refactoring library for python
http://jedi.readthedocs.io
Other
5.73k stars 503 forks source link

properties with setters are now reported as 'property' for completion #1983

Closed eirannejad closed 5 months ago

eirannejad commented 5 months ago

When complete is called on this script, current jedi reports X as a 'method' instead of a 'property'. This PR fixes that

class Point3d:
    @property
    def X(self) -> float:
        return 0.0

    @X.setter
    def X(self, value: float):
        return None

p = Point3d()
p.
eirannejad commented 5 months ago

@davidhalter I'm not sure why the integration tests are failing. Any ideas?

davidhalter commented 5 months ago

Have a look at the failed test in test/completion/context.py at line 28 and I'm sure you will understand what failed (just read the comments) :)

Thanks for the fix!

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (740b474) 94.50% compared to head (6f643cb) 94.46%.

:exclamation: Current head 6f643cb differs from pull request most recent head 990b9c9. Consider uploading reports for the commit 990b9c9 to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1983 +/- ## ========================================== - Coverage 94.50% 94.46% -0.05% ========================================== Files 80 80 Lines 11932 11932 ========================================== - Hits 11276 11271 -5 - Misses 656 661 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

davidhalter commented 5 months ago

@eirannejad No sorry, that's not what I meant. My fault, sorry. I thought it was obvious that the comment IMO this is not wanted means that the tests are wrong and not your implementation. You actually fixed something that was previously just tested in its current (unfinished) state.

eirannejad commented 5 months ago

@davidhalter Okay. I think I got that but to be honest I don't know the depth of what jedi does so I wasn't able to write new tests for this change. I modified the PR so it does not report property for the __class__ as it apparently has a .setter decorator.

I could use a little guidance on how you'd want this to work ideally so I can make the necessary changes.

davidhalter commented 5 months ago

OK, sorry. It seems I was still not clear enough. The original change you did was the right one.

However the current tests that were failing before are wrong:

    # This might or might not be what we wanted, currently properties are also
    # used like this. IMO this is not wanted ~dave.                           
    #? ['__class__']                                                          
    def __class__                                                             
    #? []                                                                     
    __class__                                                                 

This is what we currently have, but it's not what we want as I mentioned in the comment of the tests. Therefore I wanted you to change the test/completion/context.py file, remove my old comment (that says [..] IMO this is not wanted [..] and change the tests like this:

    #? []                                                          
    def __class__                                                             
    #? ['__class__']                                                                     
    __class__ 

This is what we would have wanted from the start with these tests, but without your change that was not possible. I guess I thought you would understand my comment and simply change the tests, but I can understand that this wasn't clear. Do you understand now?

eirannejad commented 5 months ago

@davidhalter I think this is ready.

davidhalter commented 5 months ago

Thanks a lot! @eirannejad