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 c language #116

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

Mephistophiles
Copy link
Contributor

No description provided.

@alexpovel
Copy link
Owner

This looks fantastic, appreciate the work! I’ll take a closer look soon but on first glance it looks great.

Two notes:

  • CI might complain about unsorted imports. If you install a nightly Cargo toolchain and run cargo formatting (on nightly) it should sort itself out, no manual work required. The pre-commit config has the command for that.
  • let’s see about bumping dependencies, that has not been without pain in the past. Ideally this PR implements C support on the current tree-sitter version, then we bump versions separately (PRs for that exist already, build(deps): bump tree-sitter from 0.22.6 to 0.23.0 #112 ). Does that work for you or do the new versions bring changes you need?

@Mephistophiles
Copy link
Contributor Author

Mephistophiles commented Sep 21, 2024

Hi! Thanks for the review!

CI might complain about unsorted imports. If you install a nightly Cargo toolchain and run cargo formatting (on nightly) it should sort itself out, no manual work required. The pre-commit config has the command for that.

Hmm, I have the same formatting on cargo-fmt 1.83. I'll look a little later to see why it doesn't format for me.

let’s see about bumping dependencies, that has not been without pain in the past

I'd like to use the latest version of tree-sitter-c, but it requires tree-sitter 0.23.
I'm still testing this parser, so feel free to merge PR with bump tree-sitter version. I'll just rebase my branch later and resolve the conflicts.

@alexpovel
Copy link
Owner

I pushed a change ( e847d37 ) that might address the CI failure at https://github.com/alexpovel/srgn/actions/runs/10971586945/job/30467773900?pr=116 in this PR. But no idea what's wrong there...

I'm looking into bumping tree-sitter versions. Not sure if tree-sitter/tree-sitter#3069 introduced changes I need to respect here. But nothing for you to worry about.

@Mephistophiles Mephistophiles force-pushed the add-support-c-and-cpp branch 2 times, most recently from 4eb8b08 to 4f603ee Compare September 22, 2024 09:01
@Mephistophiles Mephistophiles marked this pull request as ready for review September 22, 2024 09:01
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2024

Codecov Report

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

Project coverage is 86.57%. Comparing base (45b6455) to head (1178737).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/scoping/langs/c.rs 77.77% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
- Coverage   86.68%   86.57%   -0.11%     
==========================================
  Files          31       32       +1     
  Lines        1937     1974      +37     
==========================================
+ Hits         1679     1709      +30     
- Misses        258      265       +7     

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

@Mephistophiles
Copy link
Contributor Author

Hi! I rebase my branch. I hope CI won't crash now =).
Can you tell me what other things should be done when adding support for a new language? (Should I add any additional tests, documentation?)

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.

@Mephistophiles , I'd be interested in your feedback. How was working on this for you? Adding support for a new language is not documented. Were there things you stumbled over (documentation, automation, ...)?

src/main.rs Outdated Show resolved Hide resolved
src/scoping/langs/c.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Mephistophiles
Copy link
Contributor Author

@alexpovel, When adding support for the C language, I relied on the comparison of how support was implemented for other languages. So, I didn't encounter any particular problems.
I really liked that readme was also tested, this allowed me to pay attention to the fact that readme should also be updated =).

Regarding automation with pre-commit: I have NixOS, so commitizen and markdown-lint did not work well for me due to the peculiarities of nixos (linker is located in completely different paths, different from the standard /lib /usr/lib), but this is just a peculiarity of my environment.

Regarding tests: do you think it is possible to automate the launch of tests for each prepared query language? This would allow updating test files (base files) in time. And another small question for the insta snapshot engine (I haven't worked with it much), is it possible to somehow detect old snapshot files that are no longer used? It happened to me that when renaming a prepared query, old files remained in staged, only rm -rf base.c_* helped =).

@Mephistophiles
Copy link
Contributor Author

Oh, Main / Execute release chores (pull_request) still failing =( The crash couldn't be related to my commits?

@alexpovel
Copy link
Owner

alexpovel commented Sep 23, 2024

Thanks for your feedback! Regarding your points:

  • I run Nix (but on macOS). Perhaps one day I'll put a flake.nix into the repo to have everything truly fixed. I've done that in simpler cases in the past and it works fine.
  • The creation of test cases for
    fn test_language_scopers(
    could surely be automated. But not sure it's worth it: it would be macro hell, and adding support for a new language/new test cases is quite rare. The repo has test coverage to see if cases were missed.
  • You can run something like cargo insta test --unreferenced=delete to remove old snapshots. HOWEVER, that might not work with platform-dependent snapshots (example), which are a stupid hack I introduced. Their names are generated in a way insta might not recognize and delete accidentally (but then you can just git restore):
    if os_dependent {
  • The workflow Main / Execute release chores (pull_request) doesn't matter in PRs, all good :)

@alexpovel alexpovel merged commit da2580a into alexpovel:main Sep 23, 2024
14 of 15 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