jaq icon indicating copy to clipboard operation
jaq copied to clipboard

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

Open bb010g opened this issue 8 months ago • 9 comments

[!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).

bb010g avatar Dec 17 '23 07:12 bb010g

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 avatar Jan 10 '24 12:01 01mf02

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.

01mf02 avatar Jan 17 '24 10:01 01mf02

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.

bb010g avatar Jan 18 '24 03:01 bb010g

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.

01mf02 avatar Jan 18 '24 12:01 01mf02

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.

bb010g avatar Jan 28 '24 05:01 bb010g

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.

bb010g avatar Feb 22 '24 19:02 bb010g

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 avatar Mar 11 '24 14:03 01mf02

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 avatar Mar 11 '24 14:03 01mf02

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 avatar Mar 12 '24 00:03 bb010g