jupyterlab / jupyterlab

JupyterLab computational environment.
https://jupyterlab.readthedocs.io/
Other
13.81k stars 3.12k forks source link

Added >=0 check on delta #16255

Open nuvious opened 2 weeks ago

nuvious commented 2 weeks ago

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

jupyterlab-probot[bot] commented 2 weeks ago

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

welcome[bot] commented 2 weeks ago

Thanks for submitting your first pull request! You are awesome! :hugs:
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! :wave:
Welcome to the Jupyter community! :tada:

JasonWeill commented 2 weeks ago

@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 commented 2 weeks ago

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.