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.

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

Adds a "never" expiration option #125

Merged

Conversation

viktorpenelski
Copy link
Contributor

This is a naive way of solving #92 by adding a "never" expire option by setting the data way into the future.

image

@github-actions
Copy link

github-actions bot commented Mar 23, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@viktorpenelski
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@viktorpenelski viktorpenelski changed the title Adds a 'never' expiration by setting the date 999 years into the future Adds a "never" expiration by setting the date 999 years into the future Mar 24, 2022
@mtlynch
Copy link
Owner

mtlynch commented Mar 24, 2022

Cool, thanks for taking this on, @viktorpenelski!

I prefer this approach to #124 because I'm worried about a case where we forget to check for nil time in something like the garbage collector and we end up deleting all the "Never" expire files because they look like they expired.

I think the best approach is to define a var in types.go like this:

var NeverExpire = ExpirationTime(time.Date(3000, time.January, 0, 0, 0, 0, 0, time.UTC))

And then use that for the "Never" option. Then we can edit formatExpiration to special-case the Never option like:

if et == types.NeverExpire {
    return "Never"
}

I refactored the expiration option rendering to happen server side (#126) to make this easier.

@viktorpenelski
Copy link
Contributor Author

Thanks, @mtlynch, the refactor makes it really easy!

I've added the NeverExpire type alongside GC tests to hopefully catch if someone moves NeverExpire to an earlier time.

image

Out of curiosity, is there a nicer way to tie a constant variable or a method to a custom type? E.g. instead of having var NeverExpire on its own, have it tied down to ExpirationTime, like ExpirationTime.Never? (for example in Kotlin that would be easy to add with an extension property)
(sry if it is a dumb question, I'm not familiar with Golang)

@mtlynch mtlynch changed the title Adds a "never" expiration by setting the date 999 years into the future Adds a "never" expiration option Mar 24, 2022
@mtlynch
Copy link
Owner

mtlynch commented Mar 24, 2022

LGTM, thanks!

Out of curiosity, is there a nicer way to tie a constant variable or a method to a custom type? E.g. instead of having var NeverExpire on its own, have it tied down to ExpirationTime, like ExpirationTime.Never? (for example in Kotlin that would be easy to add with an extension property)
(sry if it is a dumb question, I'm not familiar with Golang)

Good question! I don't think this is possible. The closest I can think of is to tie it to a package, the way Golang does with months like time.January. So we theoretically could create a dedicated package called expirations and have a var Never so we'd have the readability of expirations.Never, but that's probably too much just for one var.

It is properly type-checked though. If you try to treat types.NeverExpire as a regular time.Time (instead of a types.ExpirationTime), compilation will correctly fail:

cannot use types.NeverExpire (type types.ExpirationTime) as type time.Time in argument to time.Now().After

@mtlynch mtlynch merged commit 5e51dfb into mtlynch:master Mar 24, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants