feat: teach alternation update-assignment operator (`//=`) #142

bb010g · 2023-12-17T07:01:37Z

Note

This PR is a patch stack. Do not squash these commits.

Implement the alternation update-assignment operator (called the "alternate update-assignment operator") by jq.

Additionally, increase documentation coverage & make some documentation more consistent.

The name AltUpdate is chosen in anticipation of renaming UpdateWith & UpdateMath to MathUpdate. If AssignOp::UpdateWith should be kept, I believe it should become UpdateWith(BinaryOp), and Ast::UpdateMath should then also both be renamed to Ast::UpdateWith and become UpdateWith(Id, BinaryOp, Id).

01mf02 · 2024-01-10T12:12:54Z

Hi @bb010g, thanks for your extensive changes and congratulations for venturing out into creating a new operator!

I made a few comments about changes that should be quite easy to make. Once you did this, I will be very happy to merge your PR.

01mf02 · 2024-01-17T10:21:25Z

On a second thought, I think it would be better if you could split this PR into two: one for your documentation additions, and one for the actual implementation of //=. The reason behind this is that I cannot merge the //= parts until jaq 2.0, because that is a breaking change for jaq-syn and thus for jaq itself. However, the other parts would be already useful to merge earlier, because they are no breaking changes.

bb010g · 2024-01-18T03:27:33Z

I made a few comments about changes that should be quite easy to make.

I don't see any of these comments.

On a second thought, I think it would be better if you could split this PR into two: one for your documentation additions, and one for the actual implementation of //=. The reason behind this is that I cannot merge the //= parts until jaq 2.0, because that is a breaking change for jaq-syn and thus for jaq itself. However, the other parts would be already useful to merge earlier, because they are no breaking changes.

Understandable! I can do so. Alternatively, could this new syntax be put behind an experimental/unstable flag of some sort? E.g. jaq --unstable-syntax on the CLI that can be stabilized along with 2.0.0.

01mf02 · 2024-01-18T12:24:48Z

I don't see any of these comments.

It's in my review: https://github.com/01mf02/jaq/pull/142/files/d9af7e7b7e3aea249ca08065410f94c93b02f7c8
I can also see these comments in this thread --- just scroll above.

Understandable! I can do so. Alternatively, could this new syntax be put behind an experimental/unstable flag of some sort? E.g. jaq --unstable-syntax on the CLI that can be stabilized along with 2.0.0.

Thanks!
Unfortunately, such a flag would not work. The problem is that adding the AltUpdate variant to jaq-syn requires a bump of jaq-syn to at least version 2.0, which requires a bump of jaq-interpret as well (because jaq-syn is a public dependency of jaq-interpret). As I mentioned in https://github.com/01mf02/jaq/releases/tag/v1.0.0, I want to keep the API (of jaq-interpret) stable as long as possible, and collect multiple breaking changes in one pass.

bb010g · 2024-01-28T05:43:16Z

Unfortunately, such a flag would not work. The problem is that adding the AltUpdate variant to jaq-syn requires a bump of jaq-syn to at least version 2.0, which requires a bump of jaq-interpret as well (because jaq-syn is a public dependency of jaq-interpret). As I mentioned in v1.0.0 (release), I want to keep the API (of jaq-interpret) stable as long as possible, and collect multiple breaking changes in one pass.

jaq-syn & jaq-interpret could learn a non-default feature unstable, which jaq could then enable.

validated with `cargo +1.63 check -p jaq-syn`.

prepare for testing the alternative update-assignment operator on the `null` input value.

jq calls this the "alternative update-assignment operator". note that jq documents the alternation operator as an arithmetic operator and that jaq does not internally represent the alternation operator as a math operator. jq 1.6 considers chained `//=` operators without parentheses a syntax error, like with `|=`; jaq allows chained `//=` operators by binding to the right, at the same precedence as `|=`.

bb010g · 2024-02-22T19:04:41Z

Okay, just pushed my first chunk of work on unstable-flag. This should be able to be released without breaking semver, with the concession that setting any unstable flag (whether in the API or the CLI) brings potential breakage, but you can enable the feature right now without causing breaking changes on the CLI. (at least, I think; I need to write up more tests).

I still need to work through where unstable: bool should be used internally, but it seems to be working alright for the use case of introducing alternation update-assignment right now. Also, I need to send patches to chumsky for an either feature and fail() function. (chumsky::primitive::custom wasn't enjoyable to work with.)

Also, I still can't see the review comments you've talked about, and neither can anyone else I've linked this PR to.

01mf02 · 2024-01-10T11:35:33Z

.github/workflows/msrv.yml

+    - name: Check jaq-syn
+      working-directory: jaq-syn
+      run: cargo check
+    - name: Check jaq-interpret
+      working-directory: jaq-interpret
+      run: cargo check


This should not be needed, because jaq-core depends on jaq-interpret, which depends again on jaq-syn.

01mf02 · 2024-01-10T11:37:36Z

jaq-syn/src/filter.rs

    And,
-    /// Arithmetic operation, e.g. `l + r`, `l - r`, ...
+    /// Arithmetical operator (`+`, `-`, `*`, `/`, `%`, …)


I would consistently use "arithmetic" instead of "arithmetical" everywhere.

01mf02 · 2024-01-10T11:38:29Z

jaq-syn/src/filter.rs

 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[derive(Clone, Debug)]
 pub enum AssignOp {
-    /// `=`
+    /// Assignment operator (`=`)


I would omit writing "operator" for every enum variant, because it is already specified in the documentation for the enum.

01mf02 · 2024-01-10T11:39:26Z

jaq-syn/src/filter.rs

    Object(Vec<KeyVal<Spanned<Self>>>),

-    /// Identity, i.e. `.`
+    /// Nullary identity operation (`.`)


Here again and in later points, I would omit the "operation".

01mf02 · 2024-01-10T11:42:48Z

jaq-interpret/tests/tests.rs

-    gives(ab(1), ".a  = (.a, .b)", [ab(1), ab(2)]);
-    gives(ab(1), ".a += (.a, .b)", [ab(2), ab(3)]);
-    gives(ab(1), ".a |= (.+1, .)", [ab(2)]);
+    gives(ab(Some(1)), ".a  = (.a, .b)", [ab(Some(1)), ab(Some(2))]);


I didn't know that you could pass an Option to the json! macro --- cool trick!

01mf02 · 2024-01-10T11:44:39Z

jaq-interpret/tests/tests.rs

+    gives(
+        ab(None),
+        ".a = .a+.b | ., .a = .a+.b",
+        [ab(Some(2)), ab(Some(4))],
+    );
+    gives(
+        ab(None),
+        ".a //= .a+.b | ., .a //= .a+.b",
+        [ab(Some(2)), ab(Some(2))],
+    );


Can you explain what is going on here? I have the impression that this test illustrates some precedence difference between = and //=, but I'm not sure.

01mf02 · 2024-01-10T12:10:23Z

jaq-interpret/src/filter.rs

+            Ast::AltUpdate(path, f) => w(f).pipe(cv, move |cv, y| {
+                w(path).update(
+                    cv,
+                    Box::new(move |x| box_once(Ok(if x.as_bool() { x } else { y.clone() }))),
+                )
+            }),


Oh, this is some evil stuff! The jq documentation specifies that a op= b is equivalent to a |= . op b, so I thought your implementation is not correct. But actually the jq documentation lies, because the stated property does not hold. Proof: {a: 1} | .a += .a yields {a: 2}, whereas {a: 1} | .a |= (. + .a) yields an error (in jq 1.6). So your implementation looks right. Perhaps it would be nice to make a few more tests for //=, especially for when the RHS of //= yields multiple elements.

01mf02 · 2024-01-17T10:22:25Z

jaq-interpret/src/filter.rs

+            Ast::Assign(path, f) => w(f).pipe(cv, move |cv, y| {
+                w(path).update(cv, Box::new(move |_| box_once(Ok(y.clone()))))
+            }),


Please do not move code without necessity; it makes it harder to analyse changes, in particular when using tools like git blame.

01mf02 · 2024-03-11T14:22:33Z

Also, I still can't see the review comments you've talked about, and neither can anyone else I've linked this PR to.

Oh, this seems to have been my fault; I did not realise that I needed to publish the review. Sorry for the confusion.

01mf02 · 2024-03-11T14:36:48Z

Okay, just pushed my first chunk of work on unstable-flag. This should be able to be released without breaking semver, with the concession that setting any unstable flag (whether in the API or the CLI) brings potential breakage, but you can enable the feature right now without causing breaking changes on the CLI. (at least, I think; I need to write up more tests).

I still need to work through where unstable: bool should be used internally, but it seems to be working alright for the use case of introducing alternation update-assignment right now. Also, I need to send patches to chumsky for an either feature and fail() function. (chumsky::primitive::custom wasn't enjoyable to work with.)

I have to admit that I am not fond of the "unstable" feature. I do not want enabling flags to break semantic versioning.

I think that your PR has exceeded a critical size. It's currently mixing several things: documentation, refactoring, adding new features.
I think that your work would benefit from dividing it into smaller PRs: one for documentation, one for refactoring, and one for adding your update alternation feature. That way, I could merge the documentation and refactoring ones, while merging the update alternation feature once the other parts for jaq 2.0 are ready.

01mf02 · 2024-03-11T14:39:49Z

jaq-interpret/src/into_iter.rs

-    L(L),
-    R(R),
-}
+pub struct Either<L, R>(pub either::Either<L, R>);


I prefer to keep my own Either type, because I hardly make use of anything that either::Either provides.

bb010g · 2024-03-12T00:31:11Z

I have to admit that I am not fond of the "unstable" feature. I do not want enabling flags to break semantic versioning.

Understandable. I haven't gotten back to this yet, but I think what unstable-flag really wants to be is a general version flag / parsing/evaluation context. You'd still need unstable/unstable-flag for internal use if you want to ship newer features on the CLI before major versions, but this could be much better expressed internally. The current state is mostly a proof of concept that you can introduce that execution context without immediately breaking semver. If we make stuff like the language version in that context a non-exhaustive enum, then we can introduce a new alternative to that on a minor release without breaking consumers. We'll still potentially want to test new language semantics under #[cfg(feature = "unstable")], but that should heavily reduce the pain.

Also, using an execution context opens up potential future support for both jaq's semantics and jq's semantics, side-by-side. I don't know if we'll ever end up implementing that, but it's a neat possibility. jaq could also support version flags in files to declare the language version for that file, and have that mix properly with different versions. Of course, that all depends on what our support range is, but I think that flexibility is the right call long-term.

I think that your PR has exceeded a critical size. It's currently mixing several things: documentation, refactoring, adding new features.
I think that your work would benefit from dividing it into smaller PRs: one for documentation, one for refactoring, and one for adding your update alternation feature. That way, I could merge the documentation and refactoring ones, while merging the update alternation feature once the other parts for jaq 2.0 are ready.

Very much agreed; I haven't split this up yet due to stuff not quite being ready yet. I'll be submitting a reorganized & split up the patch stack later.

bb010g mentioned this pull request Dec 17, 2023

refactor: AssignOp::UpdateWith → AssignOp::MathUpdate, Ast::UpdateMath → Ast::MathUpdate #143

Draft

01mf02 added this to the 2.0 milestone Jan 17, 2024

bb010g added 6 commits February 22, 2024 00:25

fix(jaq-syn): explicitly declare MSRV

759ae5e

validated with `cargo +1.63 check -p jaq-syn`.

style: normalize assignment operator sorting

a9f8b7a

docs: consistify filter & operator documentation

9c09972

test: refactor (update-)assignment operator tests

cd8f76c

prepare for testing the alternative update-assignment operator on the `null` input value.

[WIP] feat: unstable-flag feature

362b7b1

bb010g force-pushed the update-alt branch from d9af7e7 to 362b7b1 Compare February 22, 2024 18:57

bb010g marked this pull request as draft February 22, 2024 19:12

01mf02 requested changes Mar 11, 2024

View reviewed changes

01mf02 reviewed Mar 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: teach alternation update-assignment operator (`//=`) #142

feat: teach alternation update-assignment operator (`//=`) #142

bb010g commented Dec 17, 2023

01mf02 commented Jan 10, 2024

01mf02 commented Jan 17, 2024

bb010g commented Jan 18, 2024

01mf02 commented Jan 18, 2024 •

edited

bb010g commented Jan 28, 2024

bb010g commented Feb 22, 2024

01mf02 Jan 10, 2024

01mf02 Jan 10, 2024

01mf02 Jan 10, 2024

01mf02 Jan 10, 2024

01mf02 Jan 10, 2024

01mf02 Jan 10, 2024

01mf02 Jan 10, 2024

01mf02 Jan 17, 2024

01mf02 commented Mar 11, 2024

01mf02 commented Mar 11, 2024

01mf02 Mar 11, 2024

bb010g commented Mar 12, 2024

feat: teach alternation update-assignment operator (//=) #142

Are you sure you want to change the base?

feat: teach alternation update-assignment operator (//=) #142

Conversation

bb010g commented Dec 17, 2023

01mf02 commented Jan 10, 2024

01mf02 commented Jan 17, 2024

bb010g commented Jan 18, 2024

01mf02 commented Jan 18, 2024 • edited

bb010g commented Jan 28, 2024

bb010g commented Feb 22, 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

01mf02 commented Mar 11, 2024

01mf02 commented Mar 11, 2024

Choose a reason for hiding this comment

bb010g commented Mar 12, 2024

feat: teach alternation update-assignment operator (`//=`) #142

feat: teach alternation update-assignment operator (`//=`) #142

01mf02 commented Jan 18, 2024 •

edited