Skip to content
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

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

Merged
merged 10 commits into from
Feb 19, 2024

Conversation

eirannejad
Copy link
Contributor

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
Copy link
Contributor Author

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

@davidhalter
Copy link
Owner

davidhalter commented Feb 7, 2024

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
Copy link

codecov-commenter commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidhalter
Copy link
Owner

@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
Copy link
Contributor Author

eirannejad commented Feb 8, 2024

@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
Copy link
Owner

davidhalter commented Feb 8, 2024

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
Copy link
Contributor Author

@davidhalter I think this is ready.

@davidhalter
Copy link
Owner

Thanks a lot! @eirannejad

@davidhalter davidhalter merged commit 54a6dad into davidhalter:master Feb 19, 2024
114 checks passed
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.

3 participants