matze / wastebin

wastebin is a pastebin
MIT License
250 stars 25 forks source link

add maximum expiration logic #55

Open bt-nia opened 1 month ago

bt-nia commented 1 month ago

Disclaimer: I never programmed RUST and this is mostly LLM generated code

This aims to implement https://github.com/matze/wastebin/issues/54

cc: @matze @denissteinhorst for visibility

bt-nia commented 2 weeks ago

any update on this? :)

matze commented 1 week ago

I left a change request a while ago, regarding the max_expiration() function.

bt-nia commented 1 week ago

@matze can you please point me to it? I can't find it.

matze commented 1 week ago

Wait, you can't see the "Please return Option and Ok(None) here." referring to line 89 inenv.rs?

bt-nia commented 1 week ago

Not sure where I can see that? This is my change view:

image

I've never submitted change requests in Github, but I can say that in Gitlab it is easy to accidentally only submit comments for review but never actually post the review.

Thanks for sharing the change. Two comments:

  1. If I return None in line 89, can I check for None in the template in the same fashion? I expect not, which probably makes this more complicated and error-prone
  2. It seems off to me to return None when the return value is supposed to be an integer. I'm not a Rust expert, but returning None would be something I would expect in an error case not in a success case. Otherwise it sounds a bit like a JavaScript style thing.
matze commented 4 days ago

Sorry, you couldn't see it until I submitted the review :see_no_evil:

bt-nia commented 3 days ago

Sorry, you couldn't see it until I submitted the review 🙈

No problem! 🍻

I was able to test the code now and made some improvements to the index.html template. I looked into your proposal, but from what it seems to me it is mostly a complication that makes value types less predictable. If you still believe that not returning an integer here would be beneficial, please let me know as to why.

That said, unless you see any glaring issues, in which case: please let me know, could we please merge this? It is a feature we would really like to use for our deployment :)