Rename `Language` to `CompiledQuery` #151

bheylin · 2024-10-19T14:28:41Z

@alexpovel I've performed the refactorings we talked about in PR #144 and moved the handle_language_scope macro into the mod cli so that the fields are and lang::CompiledQuery's are strongly associated.

One of the CLI tests is failing so I'll mark this as WIP.

bheylin · 2024-10-19T18:23:07Z

@alexpovel I reduced the amount of code in the impl_lang_scopes macro and fixed the tests.

See: 0b21418

I'll fix the clippies tommorrow.

codecov-commenter · 2024-10-19T18:29:34Z

Codecov Report

Attention: Patch coverage is 97.26776% with 15 lines in your changes missing coverage. Please review.

Project coverage is 86.77%. Comparing base (d05f278) to head (0de07c9).

Files with missing lines	Patch %	Lines
src/main.rs	96.61%	4 Missing ⚠️
src/scoping/langs/csharp.rs	88.88%	3 Missing ⚠️
src/scoping/langs/typescript.rs	93.02%	3 Missing ⚠️
src/scoping/langs/c.rs	93.10%	2 Missing ⚠️
src/scoping/langs/hcl.rs	97.46%	2 Missing ⚠️
src/scoping/langs/mod.rs	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
+ Coverage   86.69%   86.77%   +0.08%     
==========================================
  Files          32       32              
  Lines        1969     1883      -86     
==========================================
- Hits         1707     1634      -73     
+ Misses        262      249      -13

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

bheylin · 2024-10-20T18:06:03Z

@alexpovel All clippies are fixed and the tests pass locally.

alexpovel · 2024-10-20T18:24:30Z

Thank you! I'll give it a read soon.

In CI, the feature power set test is failing, so seems like some parts of the refactor need to be gated behind feature attributes again

bheylin · 2024-10-20T19:21:33Z

Ah yea the german feature needed some attention.
I pulled the features out of fn assemble_actions in ff232f5.

bheylin · 2024-10-21T17:36:50Z

@alexpovel Do you want to allow the cognitive complexity clippy or shall I do a pass over fn main?

alexpovel · 2024-10-21T17:23:50Z

src/main.rs

+    let cli::Args {
+        scope,
+        shell,
+        composable_actions,
+        standalone_actions,
+        mut options,
+        languages_scopes,
+        #[cfg(feature = "german")]
+        german_options,
+    } = args;


Destructuring for the win!

alexpovel · 2024-10-21T17:48:52Z

tests/langs/mod.rs

 ) {
+    let lang = lang.expect("Building a Language for a test should not fail");


So there's no longer a way to statically (type level) say: custom, user-provided queries are expected to fail and if so, fail gracefully, while prepared queries are expected to be valid and would panic (but don't, as is validated through testing)?

Because under that model, try_from on a prepared query wouldn't exist.

Just asking. Not a blocker.

Good point, we can definitely do more with typing given that we know the prepared queries are syntactically valid and tested to be so.

I'll give this a look and if you like the solution I'll squash the commits.

Alright, but no worries. Such a refactor might lead down the path we came from in the first place (PhantomData, ...) 😉

And I'm not sure it was previously possible either, as the code query was an enum and not separate types.

See what you think of something like this: 766ca78.
Maybe a trait is strong enough.

We could seal the trait so that prepared queries are only definable internally.

And I clearly need to fix the macro if we continue on with this solution.

A new trait can work, but I'm wondering if the solution couldn't be quite simple:

impl From<PreparedQuery> for CompiledQuery { // ... }

would that not be enough? This skips the intermediate, fallible RawQuery state, and skips to the known-good state (and panics if necessary, but tests will show it won't). It can live in parallel with impl From<PreparedQuery> for RawQuery if that's also needed.

Just skimmed over the proposal quickly so might be off base.

And if it cannot be made simpler, I slightly prefer the previous approach, where we simply unwrap on RawQuery. A slight blip in the type system but a simpler implementation.

Using Into is indeed the simplest implementation you just end up needing to disambiguate the conversion to impl LanguageScoper in the tests.

I'll make an Into so we can compare.

alexpovel · 2024-10-21T17:55:36Z

@bheylin let's allow the cognitive complexity in main. I might refactor that function in the future anyway, the handling of the different options ([single-/multi-threaded], [read from stdin/files], [write to stdout/files], ...) is really nasty currently. But that's a future topic.

alexpovel · 2024-10-21T17:57:05Z

Once that lint is silenced I'm happy to merge. Again, feel free to squash, else I'll do it for you

bheylin · 2024-10-22T20:30:34Z

@alexpovel check out proposal 2 c0ddb6c

alexpovel · 2024-10-26T15:57:43Z

Looks great! It's much simpler than the previous proposal, so a win in my books. This reads nice and fluent for example:

c0ddb6c#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR1306-R1313

I'm happy to move forward with this 🙂

alexpovel · 2024-10-26T16:00:13Z

Oh CI is green, didn't expect that (thought you were still experimenting). So it this good to go from your perspective, save for some cleanup?

…ete types - Merged the `Language` and `CodeQuery` aliases into a single `CompiledQuery` type per language mod `c::CompiledQuery`. - Rename the `Preparred[lang]Query` to `[lang]::ProparredQuery`. Leave the namespace disambiguate the type name. - Add a direct conversion between the `[lang]::PreparedQuery` and `[lang::CompiledQuery]`. - Rename `cli::Cli` to 'cli::Args'. - Use destructuring and object moving to avoid needless cloning and reference passing of the large `cli::Args` object. Destructuring allows us to pass the destuctured subobjects of `cli::Args` into the various processing functions so the reader can see clearly the dependencies of each function. - Add `enum StandaloneAction` to replace the difficult to read literal bool in pattern matching. - Replace the `macro_rules! handle_language_scope` with a `macro_rules impl_lang_scopes!` that links the `struct cli::LanguageScopes` and the list of `dyn LanguageScopes` used to perform tree-sitter matches. This results in less code in the `macro_rules!` and more invariants enforced by the type system.

bheylin · 2024-10-27T08:38:55Z

@alexpovel I've squashed the commits and written a detailed commit message.
This PR is ready to go as far is I'm concerned.

alexpovel · 2024-10-27T10:08:14Z

Sounds good, merged. Thank you for all your work so far!

bheylin · 2024-10-27T10:58:32Z

Sounds good, merged. Thank you for all your work so far!

Great! Thanks for your time and feedback.

I'm going to make another smaller cleanup PR around renaming the trait LanguageScoper.
Then I'll rebase #144 and continue on with the feature.

bheylin marked this pull request as ready for review October 20, 2024 18:04

bheylin changed the title ~~Rename lang to compiled query~~ Rename Language to CompiledQuery Oct 20, 2024

bheylin force-pushed the rename-lang-to-compiled-query branch 2 times, most recently from 212ca28 to ff232f5 Compare October 20, 2024 19:19

alexpovel approved these changes Oct 21, 2024

View reviewed changes

bheylin force-pushed the rename-lang-to-compiled-query branch 3 times, most recently from 16e00fd to e0b0382 Compare October 27, 2024 08:34

bheylin force-pushed the rename-lang-to-compiled-query branch from e0b0382 to 0de07c9 Compare October 27, 2024 08:36

alexpovel merged commit b53bf21 into alexpovel:main Oct 27, 2024
15 checks passed

bheylin deleted the rename-lang-to-compiled-query branch October 27, 2024 10:59

alexpovel mentioned this pull request Oct 27, 2024

Add support cpp parser #134

Draft

bheylin mentioned this pull request Oct 29, 2024

--[lang]-query can read tree-sitter (scm) query files #144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `Language` to `CompiledQuery` #151

Rename `Language` to `CompiledQuery` #151

bheylin commented Oct 19, 2024

bheylin commented Oct 19, 2024

codecov-commenter commented Oct 19, 2024 •

edited

Loading

bheylin commented Oct 20, 2024

alexpovel commented Oct 20, 2024 •

edited

Loading

bheylin commented Oct 20, 2024

bheylin commented Oct 21, 2024

alexpovel Oct 21, 2024

bheylin Oct 21, 2024

alexpovel Oct 21, 2024

bheylin Oct 21, 2024

alexpovel Oct 21, 2024

bheylin Oct 21, 2024

bheylin Oct 21, 2024

alexpovel Oct 21, 2024

alexpovel Oct 21, 2024

bheylin Oct 22, 2024

alexpovel commented Oct 21, 2024

alexpovel commented Oct 21, 2024

bheylin commented Oct 22, 2024

alexpovel commented Oct 26, 2024 •

edited

Loading

alexpovel commented Oct 26, 2024

bheylin commented Oct 27, 2024

alexpovel commented Oct 27, 2024

bheylin commented Oct 27, 2024

		) {
		let lang = lang.expect("Building a Language for a test should not fail");

Rename Language to CompiledQuery #151

Rename Language to CompiledQuery #151

Conversation

bheylin commented Oct 19, 2024

bheylin commented Oct 19, 2024

codecov-commenter commented Oct 19, 2024 • edited Loading

Codecov Report

bheylin commented Oct 20, 2024

alexpovel commented Oct 20, 2024 • edited Loading

bheylin commented Oct 20, 2024

bheylin commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexpovel commented Oct 21, 2024

alexpovel commented Oct 21, 2024

bheylin commented Oct 22, 2024

alexpovel commented Oct 26, 2024 • edited Loading

alexpovel commented Oct 26, 2024

bheylin commented Oct 27, 2024

alexpovel commented Oct 27, 2024

bheylin commented Oct 27, 2024

Rename `Language` to `CompiledQuery` #151

Rename `Language` to `CompiledQuery` #151

codecov-commenter commented Oct 19, 2024 •

edited

Loading

alexpovel commented Oct 20, 2024 •

edited

Loading

alexpovel commented Oct 26, 2024 •

edited

Loading