jaq
jaq copied to clipboard
feat: teach alternation update-assignment operator (`//=`)
[!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)
.
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.
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.
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 forjaq-syn
and thus forjaq
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.
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.
Unfortunately, such a flag would not work. The problem is that adding the
AltUpdate
variant tojaq-syn
requires a bump ofjaq-syn
to at least version 2.0, which requires a bump ofjaq-interpret
as well (becausejaq-syn
is a public dependency ofjaq-interpret
). As I mentioned inv1.0.0
(release), I want to keep the API (ofjaq-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.
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.
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.
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 aneither
feature andfail()
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.
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.