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

Added >=0 check on delta #16255

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nuvious
Copy link

@nuvious nuvious commented Apr 26, 2024

References

Resolves #16254

Code changes

Added check on delta to see if it is positive. If it is, return 0 for value and 'seconds' for the unit per documentation below and convention demonstrated at line 48/52 (current/proposed time.ts implementation).

Without this check if delta is greater than 0, Math.ceil(delta / unit.milliseconds) will compute to 1 with the units being "years" in the first check resulting in the behavior outlined in #16254

User-facing changes

User will se 'now' or '0 seconds'. Had trouble building/installing jupterlab from source in a fresh venv and still trying to so I can provide manual testing validation

Backwards-incompatible changes

N/A

Added check on delta to see if it is positive. If it is, return 0 for value and 'seconds' for the unit per documentation below and convention demonstrated at line 48/52  (current/proposed time.ts implementation).

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/RelativeTimeFormat/format
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

Copy link

welcome bot commented Apr 26, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@JasonWeill
Copy link
Contributor

@nuvious Thank you so much for opening this issue and pull request! It is possible, using commands like touch, to manually set a date in the future for a particular file. It looks like your case covers a situation where the delta is only slightly larger than 0 (by a second or so) and is being misreported as 1 year from now. There are other ways to handle this:

  1. Treat files with a modified date within a few seconds of now as "now" (common on many hosted software products)
  2. Revising the logic around the ceil function to handle cases where the file's datestamp is in the future.

I don't think that treating all future-dated files as being modified "now" is the best approach to address #16254.

It would also be good to improve the unit test coverage for future-dated files.

@nuvious
Copy link
Author

nuvious commented Apr 26, 2024

I think it is reasonable to just treat anything resolving to the future as "now". This is a narrow edge case and the value is "Last Modified" which implies a context in the past. Last modified should never be in the future because that doesn't make sense in any context. The >=0 check acts as an input validation for the ceiling function for this edge case. That's all that's really needed here is some input validation.

I can try my hands at the unit tests but need to familiarize myself with the backend a bit. Will look into that when I get a chance. Also have started using the docker/start.sh environment builder as well to do manual testing and should be able to provide that info in the near future.

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.

Date shows "next year" when creating or saving a notebook
3 participants