Skip to content

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

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

Merged
merged 18 commits into from
Nov 21, 2023

Conversation

Titou325
Copy link
Contributor

@Titou325 Titou325 commented Nov 9, 2023

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:

  • Add additional test cases covering specifics (decorators, type hints, stubs) @Titou325
  • Check for possible regressions due to updating scip.proto
  • Ensure functionality works as designed (ping @varungandhi-src) for the Python language
  • add # format-options: showDocs to other test snapshot inputs and regenerate expected outputs (currently blocked by Baseline snapshot test failures on macOS #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
Copy link
Member

Thanks @Titou325

@varungandhi-src go any feedback?

Copy link
Contributor

@varungandhi-src varungandhi-src left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had the time to look at this fully, but I recommend first changing the snapshot output format to be something more digestible.

@Titou325
Copy link
Contributor Author

Titou325 commented Nov 16, 2023

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

This PR is ready for review but will require subsequent modifications to merge into main, although they are currently blocked by #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.

Copy link
Contributor

@varungandhi-src varungandhi-src left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think this a good improvement (new feature + better testing), so thank you for the work!

I've left some style-related comments which I think would help make the code easier to follow. Most of the them are non-blocking, but it'd be nice to at least simplify the enclosing ranges logic in the snapshot code to some extent.

Tentatively approving. Let me know if you'd like to update the snapshots on Ubuntu and push (or you'd like me to do that).

@varungandhi-src
Copy link
Contributor

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

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!

@Titou325 Titou325 marked this pull request as ready for review November 20, 2023 12:41
@varungandhi-src
Copy link
Contributor

varungandhi-src commented Nov 21, 2023

@Titou325 I've updated the snapshots here using a Linux runner: codemuse-app#1

@Titou325
Copy link
Contributor Author

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 varungandhi-src merged commit 1572f2f into sourcegraph:scip Nov 21, 2023
@varungandhi-src varungandhi-src deleted the feature-86 branch November 21, 2023 11:30
@varungandhi-src
Copy link
Contributor

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

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

@maks-ivanov
Copy link

Thanks @Titou325 awesome work

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.

4 participants