Skip to content

Conversation

@brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Jun 26, 2024

Description

Examples

builder { }
seq { }

Checklist

  • Test cases added.
  • Release notes entry updated.

Important

Shoutout to @edgarfgp @fabulous-dev for sponsoring my work on this.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 26, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.400.md
LanguageFeatures.fsi docs/release-notes/.Language/preview.md

@vzarytovskii
Copy link
Member

Haven't reviewed it yet, but thinking out loud:

How would this play with resumable state machines. I assume it should be fine since it's just a transformation essentially.

And second, orthogonal comment - this feature will encourage more CE use, which my itself is not bad, but might lead to more nested CEs, which will involve more checking = being slower. We really need to do something with it. I made an attempt or two in the past, but change surface was bigger than I anticipated.

@brianrourkeboll
Copy link
Contributor Author

How would this play with resumable state machines. I assume it should be fine since it's just a transformation essentially.

That's a good call-out. I'll add some tests to make sure.

@psfinaki
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Contributor

Cool stuff! Hope to give it a review soon. Can be a nice and intuitive addition to the language.

* The warning caused by dotnet#12038
  distracted from the test's intent. Wrapping the code in a function
  removes the warning.
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Nice work, great testing. I am not a CE guru (yet) so letting @vzarytovskii to make a final review here.

Copy link
Contributor

@edgarfgp edgarfgp left a comment

Choose a reason for hiding this comment

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

Thanks @brianrourkeboll. Awesome work

@T-Gro T-Gro merged commit 5c6d8e7 into dotnet:main Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants