#86 - Adds enclosing ranges to function and class definitions

sourcegraph / scip-python

SCIP indexer for Python

Other

46 stars 21 forks source link

#86 - Adds enclosing ranges to function and class definitions #132

Closed Titou325 closed 7 months ago

Titou325 commented 7 months ago

This is a first attempt at implementing enclosing ranges for function and class definitions, closing #86.

I have attempted to keep it as simple as possible, passing a parent node if needed to pushNewOccurrence.

This does not implement enclosing ranges for reference occurences.

These are the pending items:

[x] Add additional test cases covering specifics (decorators, type hints, stubs) @Titou325
[x] Check for possible regressions due to updating scip.proto
[x] Ensure functionality works as designed (ping @varungandhi-src) for the Python language
[x] add # format-options: showDocs to other test snapshot inputs and regenerate expected outputs (currently blocked by #91)
[ ] Update documentation?

Any feedback is very welcome. Until we can update the remaining snapshots at the very least, this PR will stay in draft. Thanks!

jdorfman commented 7 months ago

Thanks @Titou325

@varungandhi-src go any feedback?

Titou325 commented 7 months ago

Hi @varungandhi-src,

Thanks for the feedback. I have updated the snapshot format to match your expectations, with the difference of the start enclosing range being a down-pointing caret. It seems to me that this would be most natural given that the start range information is given above the first line (due to possible decorators presence), while the end range is below the last line of the range.

This PR is ready for review but will require subsequent modifications to merge into main, although they are currently blocked by #91.

varungandhi-src commented 7 months ago

This PR is ready for review but will require subsequent modifications to merge into main, although they are currently blocked by https://github.com/sourcegraph/scip-python/issues/91

We don't need to be blocked by that issue. Either I or you can run the snapshots on Ubuntu and push that once the code changes are done.

varungandhi-src commented 7 months ago

Check for possible regressions due to updating scip.proto

scip.proto evolves in a backwards compatible way, so it shouldn't affect indexer output.

Titou325 commented 7 months ago

Hi!

Updated the PR regarding all points above mentioned. I don't have a Linux runner easily accessible at this time, would be glad if you could update the snapshots @varungandhi-src. N.B. the showDocs option will probably be required on the other files.

Thanks a lot for the feedback!

varungandhi-src commented 7 months ago

@Titou325 I've updated the snapshots here using a Linux runner: https://github.com/codemuse-app/scip-python/pull/1

Titou325 commented 7 months ago

Hi @varungandhi-src, I have merged and updated the PR, fixing the issue with duplicate class declarations. This is an org owned fork, so I am unfortunately unable to give blanket permissions but I have given you write access.

The snapshots should be updated on Linux runners and then I believe we are good to go. Thanks!

varungandhi-src commented 7 months ago

@Titou325 if you'd like, I think you should be able to remove my maintain access on your fork, and use this option for future PRs.

Titou325 commented 7 months ago

Perfect, thanks! Can we bump versions to trigger a new release?

maks-ivanov commented 6 months ago

Thanks @Titou325 awesome work