Skip to content

Conversation

@ramonsmits
Copy link
Member

No description provided.

@ramonsmits ramonsmits self-assigned this Oct 5, 2023
@ramonsmits ramonsmits marked this pull request as ready for review October 9, 2023 07:08
Comment on lines 601 to 602
msg.Status = args.Unresolved
delete msg['@metadata']['@expires']
{ExpirationManager.DeleteExpirationFieldScript}
Copy link
Member

Choose a reason for hiding this comment

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

From the examples on https://ravendb.net/docs/article-page/6.0/csharp/client-api/operations/patching/set-based it looks like the statements inside the update block are going to need semicolons

@DavidBoike
Copy link
Member

I'm a little concerned that the effort to try to isolate expiration behind an ExpirationManager is wasted and makes things more complex, not less. If you could do it cleanly, that'd be one thing, but there are 3 different ways to enable expiration and 2 different ways to cancel it, some of which work on the session and others that have to alter some javascript command string. It just seems really messy and error-prone.

My vote would be to revert the last 2-ish commits.

@tmasternak
Copy link
Member

@DavidBoike I think there is still value in keeping all code connected to RavenDB 5 expiry management in a single class. My reasoning is:

  • it makes it visible that expiration management is messy - removing ExpirationManager would hide this and not solve it
  • it is a single place in which all the messiness is contained - a go-to place to figure out what the logic is and were it is used.

@DavidBoike DavidBoike merged commit 468e034 into master Oct 10, 2023
@DavidBoike DavidBoike deleted the expiration branch October 10, 2023 15:40
@DavidBoike DavidBoike added this to the 5.0.0 milestone Nov 30, 2023
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.

4 participants