Skip to content

add maximum expiration logic#55

Closed
bt-nia wants to merge 5 commits intomatze:masterfrom
bt-nia:master
Closed

add maximum expiration logic#55
bt-nia wants to merge 5 commits intomatze:masterfrom
bt-nia:master

Conversation

@bt-nia
Copy link
Copy Markdown

@bt-nia bt-nia commented Jun 3, 2024

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

This aims to implement #54

cc: @matze @denissteinhorst for visibility

@bt-nia
Copy link
Copy Markdown
Author

bt-nia commented Jun 21, 2024

any update on this? :)

@matze
Copy link
Copy Markdown
Owner

matze commented Jun 24, 2024

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

@bt-nia
Copy link
Copy Markdown
Author

bt-nia commented Jun 25, 2024

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

@matze
Copy link
Copy Markdown
Owner

matze commented Jun 27, 2024

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

@bt-nia
Copy link
Copy Markdown
Author

bt-nia commented Jun 28, 2024

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 matze self-requested a review July 1, 2024 18:14
Comment thread src/env.rs Outdated
@matze
Copy link
Copy Markdown
Owner

matze commented Jul 1, 2024

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

@bt-nia
Copy link
Copy Markdown
Author

bt-nia commented Jul 2, 2024

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 :)

@bt-nia bt-nia requested a review from matze July 4, 2024 15:19
Comment thread src/env.rs Outdated
@bt-nia bt-nia requested a review from matze July 5, 2024 11:48
@HeapUnderfl0w HeapUnderfl0w mentioned this pull request Jul 6, 2024
@bt-nia bt-nia closed this Jul 10, 2024
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