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

Note that if..else now supports DML #6660

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Note that if..else now supports DML #6660

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e225d0
Note that if..else now supports DML
raddevon Jan 3, 2024
ecc224d
Clarify the workaround is for earlier versions
raddevon Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/edgeql/for.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,14 @@ A similar approach can be used for bulk updates.
Conditional DML
---------------

.. versionadded:: 4.0

DML is now supported in ``if..else``. The method of achieving conditional
DML demonstrated below is a workaround for earlier versions of EdgeDB
before this support was introduced in EdgeDB 4.0. If you're on EdgeDB 4.0
or higher, use :eql:op:`if..else` for a cleaner way to achieve conditional
DML.

DML (i.e., :ref:`insert <ref_eql_insert>`, :ref:`update <ref_eql_update>`,
:ref:`delete <ref_eql_delete>`) is not supported in :eql:op:`if..else`. If you
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads kinda of contradictory still. We're saying it is now supported in >= 4.0 but then saying it is not supported at all. I know the logic works out fine still, but maybe we should mention that it wasn't supported prior to 4.0 to make it super clear that this whole section can be ignored if you are using >= 4.0?

As an aside: I wonder if there is a section of the docs that we should update to show a demonstration of conditional DML as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. This was my argument against this model of docs versioning we've gone with. By allowing only for adding new sections to the docs as we release new versions, we end up with docs that have to contradict themselves. I do like your idea for reworking this to make that clearer though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add an example to the if..else docs where we mention this is now possible. There are probably some other places it could go too. We could mention it in unless conflict since that is itself a way to do conditional DML.

For this PR though, I'll focus on getting this one change shored up, and we can look at other opportunities to show conditional DML as a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is clearer. Thanks for the suggestion, @scotttrinh!

need to do one of these conditionally, you can use a ``for`` loop as a
Expand Down
Loading