Skip to content

feat: human readable options, and a sane default #216

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ma-ve
Copy link
Contributor

@Ma-ve Ma-ve commented Aug 4, 2025

add expiryChoicesHuman, remove 'default expiry' from dropdown

Focussing on front-end UI, 'default expiry' means nothing. Rather than relying on non-human readable, unix timestamps, introducing human readable options: (s)econds, (m)inutes, (h)ours, (d)ays. The defaultExpiryHuman should be any of the options for expiryChoicesHuman. If it's not, it'll fallback to null, which will fallback to the defaultExpiry behaviour as it is now

Save the selected option as a cookie, only after initially loaded. Load from cookie on page load. API is not changed, still accepts seconds


I've seen a (few?) discussions on the default expiry being confusing, and I concur. This is my attempt to deal with that. I can't oversee the changes required for cli tool. I also am very aware that this change is backwards compatible, but picking this over the other introduces a breaking change.

I've chaos-monkey tested this a lot: I've not been able to break this, by removing the cookie, setting an illegal value, etc.

Let me know what you think!

Ma-ve added 3 commits August 4, 2025 13:00
Focussing on front-end UI, 'default expiry' means nothing. Rather than relying on non-human readable, unix timestamps, introducing human readable options: (s)econds, (m)inutes, (h)ours, (d)ays. The defaultExpiryHuman should be any of the options for expiryChoicesHuman. If it's not, it'll fallback to `null`, which will fallback to the `defaultExpiry` as it is now

Save the selected option as a cookie, only after initially loaded. Load from cookie on page load. API is not changed, still accepts seconds
@Luzifer
Copy link
Owner

Luzifer commented Aug 4, 2025

Hm, why add new configuration options and basically duplicate the code for expiries?

The customizations are only set by a systems operator, and there is already a translation from seconds to human-readable in the frontend. Now we do have code to translate from seconds to human-readable and code to translate from human-readable to seconds and two backend options basically doing the same. That's too confusing… From an ops perspective, I have these questions in my head: Which one do I set now? Why are there two? Don't they do the same?

If you just want to have the expiry choices in a human way, you could just use []time.Duration which in the backend will automatically get parsed (of course, only with seconds, minutes, or hours but not days) but that would be a breaking change. I'm not sure whether a breaking change for a backend configuration to be human-readable can be justified.

I'm in for just selecting the first of the given choices and dropping the "default" option if it causes that much trouble. Likewise, I also like storing the choice, but please use LocalStorage instead of cookies (see the set-color-scheme key for storing the color scheme - of course this can be done in the component and does not have to be done in the index.html).

For the size formatting: Did you test JS doesn't do stupid stuff like Python creating 1.000000000000000001 with that change? Also, I'm not sure about this: 64.0 Mi states "it's point-zero," while 64 Mi could also be 64.49 Mi from a reader perspective… …not sure whether that's something "normal" people (so other than me) assume… 🤔 Personally, I don't like precision values to "jump" (so sometimes have decimal places, sometimes not).

@Ma-ve Ma-ve marked this pull request as draft August 6, 2025 13:28
@Ma-ve
Copy link
Contributor Author

Ma-ve commented Aug 6, 2025

I should have clarified: rather than see this as a feature-complete ready-to-merge PR, this was my suggestion to streamline the setup and admin-side of things, with full support for the human readable format: as a working end product

Are you open to the human-readable setup format rather than unix timestamps? If so, I reckon I could add support for both in the same config setting (all it would change is int[] to string[] in the schema, not sure if that is backwards incompatible?).

I'm in for just selecting the first of the given choices and dropping the "default" option if it causes that much trouble

I'd still prefer being able to specify a certain default, rather than the first one. Just gives the implementing party more freedom to do as they want, without it being too much of a bother maintenance wise.

re: cookies: I'll swap the cookie setup for localStorage, absolutely.

not sure whether that's something "normal" people (so other than me) assume… 🤔

Hahah, I reckon not a lot of people even use MiB rather than the plain old MB. The world is already messed up with hardware vendors selling 500 GB drives, and Windows interpreting that as ~465 GB. Personally, I'd prefer to see 64 MB or 40.5 MB even, rather than 64 MiB (=67.something MB), as that is something everyone should get.

In my case, I've used this service before to share secrets with clients, and even as they'd barely be able to make a distinction between a mouse and a keyboard, they would know what a megabyte is, and how that works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants