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

Add support cpp parser #134

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mephistophiles
Copy link
Contributor

No description provided.

@Mephistophiles
Copy link
Contributor Author

Whoops.
A commit from a neighboring branch got here.
I think this PR will be in draft before the PR #116 is merged.

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 8 lines in your changes missing coverage. Please review.

Project coverage is 86.47%. Comparing base (9238fc1) to head (b8be840).

Files with missing lines Patch % Lines
src/scoping/langs/cpp.rs 81.39% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
- Coverage   86.58%   86.47%   -0.11%     
==========================================
  Files          32       33       +1     
  Lines        1975     2019      +44     
==========================================
+ Hits         1710     1746      +36     
- Misses        265      273       +8     

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

@Mephistophiles Mephistophiles force-pushed the add-support-cpp branch 2 times, most recently from 1417869 to 12accc0 Compare September 22, 2024 18:31
@alexpovel
Copy link
Owner

#116 merged 🎉 Thanks again for your contribution! Feel free to rebase this.

Copy link
Owner

@alexpovel alexpovel left a comment

Choose a reason for hiding this comment

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

I realize this is a draft, but left some comments already!

README.md Outdated

Try to rename a class:

```cpp file=example.cpp
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps you can find a more creative example (and file name) 😄 An operation like "rename class" can already be performed very well using IDEs. srgn is more meant for things IDEs, sed etc. cannot already easily perform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to make a more useful example. I did the best I could 😆

README.md Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Looks good! I'm not familiar with C++. Feel free to go wild here. In all the other base files, I tried to put literally every single component the respective languages offer. You can for example take base.py, and hand it together with a C++ grammar definition to an LLM and ask it for a similar file, but for C++.

The reason as much as possible should be covered is to just have maximum confidence in terms of test coverage, and true/false positive matches. It guards against regressions in tree-sitter.

This is not a must-have though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the example: the example was generated by LLM. I'm not sure it's a complete set of cpp features (I'm more specialized in C language) =).

@Mephistophiles Mephistophiles force-pushed the add-support-cpp branch 2 times, most recently from 47937db to fa37d1c Compare October 6, 2024 15:05
@alexpovel
Copy link
Owner

Thank you very much! The revised example for the README looks good, at least like something you might not find in IDEs.

Let me know when you're ready to move the PR out of draft.

@alexpovel
Copy link
Owner

Just a heads-up, work in this PR will conflict with #144 . Currently I prefer merging #144 before this C++ PR. If that will end up happening, I'll fix the conflicts for you, so no worries. (but feel free to fix it yourself if you like... #144 will make implementing languages easier anyway)

@Mephistophiles
Copy link
Contributor Author

I think it is better to merge this PR after #144. After that I will fix the conflicts myself. And so maybe I'll come up with a more useful example :).

@alexpovel
Copy link
Owner

Hi @Mephistophiles !

#144 was split off into #151 , which is now merged into main and contains the language refactors. So feel free to refactor your PR onto main now.

And for the example you now have, I like it!

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