Skip to content

Conversation

nilsauf
Copy link
Contributor

@nilsauf nilsauf commented Dec 13, 2024

This PR implements a new TakeUntil operator with CancellationToken, as proposed in #2181

@nilsauf
Copy link
Contributor Author

nilsauf commented Dec 16, 2024

I just realized that there are a lot of whitespace changes. This is because the .editorconfig enforced filescoped namespaces on saving. Should I revert it back to blockscoped namespace?

@idg10
Copy link
Collaborator

idg10 commented Sep 2, 2025

My apologies for the delay in getting to this. I ended up being very busy with customer work for the first part of this year, and have only just got back to working through the backlog of work. (I don't do Rx maintenance full time)

So there are two main issues here:

  1. you've now got a merge conflict, so I can't merge this PR
  2. you decided to replace the old-style block namespace syntax with the new one in the files you modified

That 2nd point is the main reason you now have a merge conflict. Because you changed namespace style, Git thinks you changed almost every line of the files you modified.

While I do want to move over to the new style at some point, I don't want a mix of styles in the repo, so when we do move over to the new style, I'll do it with a single PR that does the whole repo.

I realise that since 9 months has passed since you submitted this, you might no longer have any time available to work on this, but if you can, could you retain the current namespace style (block-style, not the new style), which I think will have the side effect of getting rid of the merge conflict.

We'll be doing a new 6.1 release soon because I recently merged another change that adds an API, and I'd like to get this change into that as well if possible. If you don't have time to do anything with this, please let me know whether you'd like:

  1. me to copy your changes onto a new branch and merge them
  2. these change not to be incorporated

using System.Threading;

/// <summary>
/// Relays items to the downstream until the CancellationToken is cancled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type "cancled" -> "cancelled".

@nilsauf
Copy link
Contributor Author

nilsauf commented Sep 2, 2025

Thanks for looking into it!

Unfortunately I am on vacation the next 10 days and I have no access to a pc.
So if you want to do the 6.1 release after that, I can work in your fixes. Otherwise it would be nice if you can pu them in.

To the namespace issue: Reading my last comment, I think the .editorconfig of the solution forced the files copied namespace on saving. But maybe it was my personal visual studio settings? I can't verify that right now.

@idg10 idg10 changed the base branch from main to feature/takeuntil-update September 3, 2025 05:15
@idg10
Copy link
Collaborator

idg10 commented Sep 3, 2025

I think the .editorconfig of the solution forced the files copied namespace on saving. But maybe it was my personal visual studio settings?

I think it's more likely to be that second one. We didn't have any settings in the .editorconfg regarding namespace formatting, because that newer namespace syntax was introduced after we. I just added it.

Here's what I'm going to do. I'm going to adjust this PR to merge into a different branch, and then make the changes I think it needs, so we can get this in.

@idg10 idg10 deleted the branch dotnet:feature/takeuntil-update September 3, 2025 05:18
@idg10 idg10 closed this Sep 3, 2025
@idg10 idg10 reopened this Sep 3, 2025
@idg10 idg10 merged commit 9c6338c into dotnet:feature/takeuntil-update Sep 3, 2025
4 of 5 checks passed
@idg10
Copy link
Collaborator

idg10 commented Sep 3, 2025

I realised we were also missing the IQbservable version of this. (The tooling for generating the IQbservable versions needs some updates, so this process has more manual work than it's supposed to. I've done this in the new PR.) Also, we needed to update the API approval test's verified API. (Those tests ensure that we never make accidental changes to the public-facing API.)

This is all in place now in #2222 on top of your original work.

idg10 added a commit that referenced this pull request Sep 3, 2025
Based on original PR #2182 (Implementing TakeUntil with CancellationToken) by @nilsauf but with the following changes

* Revert to block-scoped namespaces
* Fix typos, standardized spelling of cancelled
* Update release notes
* Add new TakeUntil to verified API
* Add IQbservable form of TakeUntil
* Fix exception documentation on new and one existing TakeUntil overload

---------

Co-authored-by: Nils Aufschläger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants