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.

Sign up for GitHub

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

Jump to bottom

Support GENERATE * EXCEPT functionality #99

Merged
merged 2 commits into from
Apr 15, 2024
Merged

Support GENERATE * EXCEPT functionality #99

merged 2 commits into from
Apr 15, 2024

Conversation

KingMob
Copy link
Contributor

@KingMob KingMob commented Apr 10, 2024

No description provided.

@KingMob KingMob changed the title Add GENERATE * EXCEPT capability Support GENERATE * EXCEPT functionality Apr 10, 2024
sritchie
sritchie reviewed Apr 10, 2024
View reviewed changes
src/inferenceql/query/parser/tree.cljc
@@ -123,6 +123,7 @@
(walk/postwalk remove-ws node)))

(defmacro match
"Like core.match/match, but removes whitespace nodes before matching."
[vars & clauses]
(let [match (macrovich/case :clj 'clojure.core.match/match
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the cljs compiler handles this resolution on its own. If you have clojure.blah and that's not found, cljs will try to find cljs.blah. That's why (:require [clojure.string :as ]) works in cljs, for example.

Copy link
Contributor Author

@KingMob KingMob Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've never seen macrovich before, so I looked up what it's doing.

Apparently, the intent ofmacrovich/case is for Clojurescript macros. Since Clojurescript macros are technically Clojure code, reader conditionals in them will pick the wrong environment, but not this, somehow.

I don't know what was happening before, but the commit message for the change, bdefd36, says fix(cljs): Get ClojureScript working again, and the diff shows it was relying on cljs's backup loading scheme. Maybe that wasn't working properly, since it was inside macro? I don't know, but Zane might.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay! And I know I'm commenting outside of this PR... @zane do you remember what the error was that this addresses?

sritchie
sritchie approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@sritchie sritchie left a comment

Choose a reason for hiding this comment

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

This looks fine though I am NOT the ideal reviewer for bnf grammar changes... is there some test you might add that checks the new behavior of the matcher?

Otherwise :shipit:

@KingMob
Copy link
Contributor Author

KingMob commented Apr 11, 2024

This looks fine though I am NOT the ideal reviewer for bnf grammar changes... is there some test you might add that checks the new behavior of the matcher?

Otherwise :shipit:

@sritchie The grammar matching should be tested by the strict/permissive parser_test files. Was there another test you wanted to see?

@KingMob KingMob force-pushed the feature/star-except branch 2 times, most recently from 6e8700b to 90cfd70 Compare April 11, 2024 10:32
@KingMob KingMob force-pushed the push-rqxkyqrrsmrw branch from d100961 to 1fb8a6c Compare April 11, 2024 11:16
sritchie
sritchie approved these changes Apr 11, 2024
View reviewed changes
Copy link
Contributor

@sritchie sritchie left a comment

Choose a reason for hiding this comment

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

:shipit:

src/inferenceql/query/parser/tree.cljc
@@ -123,6 +123,7 @@
(walk/postwalk remove-ws node)))

(defmacro match
"Like core.match/match, but removes whitespace nodes before matching."
[vars & clauses]
(let [match (macrovich/case :clj 'clojure.core.match/match
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay! And I know I'm commenting outside of this PR... @zane do you remember what the error was that this addresses?

@sritchie
Copy link
Contributor

@KingMob looks great, no extra tests needed if those branches are covered!

@KingMob KingMob force-pushed the push-rqxkyqrrsmrw branch from 1fb8a6c to 5a5786b Compare April 12, 2024 09:06
@KingMob KingMob force-pushed the feature/star-except branch from 90cfd70 to f7adbed Compare April 12, 2024 09:06
@KingMob
Copy link
Contributor Author

KingMob commented Apr 14, 2024

@sritchie How do we examine code coverage results? I'm not experienced with these tools.

I clicked thru the coverage check in GH Actions, found a line in the logs saying the report was uploaded, and to go here, but it said the reports are unusable.

@KingMob KingMob force-pushed the push-rqxkyqrrsmrw branch from 5a5786b to 909b81d Compare April 15, 2024 15:07
@KingMob KingMob force-pushed the feature/star-except branch from f7adbed to 3823297 Compare April 15, 2024 15:07
@codecov Codecov
Copy link

codecov bot commented Apr 15, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 75.60%. Comparing base (95352b2) to head (013d3ed).

Files Patch % Lines
src/inferenceql/query/plan.cljc 69.56% 3 Missing and 4 partials ⚠️
src/inferenceql/query/literal.cljc 88.88% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
- Coverage   75.87%   75.60%   -0.28%     
==========================================
  Files          30       30              
  Lines        1484     1496      +12     
  Branches       60       64       +4     
==========================================
+ Hits         1126     1131       +5     
- Misses        298      301       +3     
- Partials       60       64       +4

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

Base automatically changed from feature/star-except to main April 15, 2024 15:19
@KingMob KingMob force-pushed the push-rqxkyqrrsmrw branch from 909b81d to 013d3ed Compare April 15, 2024 15:23
@KingMob KingMob merged commit 1e7e2dd into main Apr 15, 2024
4 of 6 checks passed
@KingMob KingMob deleted the push-rqxkyqrrsmrw branch April 15, 2024 15:35
@KingMob KingMob restored the push-rqxkyqrrsmrw branch April 15, 2024 15:36
@KingMob KingMob deleted the push-rqxkyqrrsmrw branch April 15, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sritchie sritchie sritchie approved these changes

@zane zane Awaiting requested review from zane

@littleredcomputer littleredcomputer Awaiting requested review from littleredcomputer

@Schaechtle Schaechtle Awaiting requested review from Schaechtle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KingMob @sritchie