-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
base: main
Are you sure you want to change the base?
Added >=0 check on delta #16255
Conversation
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
Thanks for making a pull request to jupyterlab! |
Thanks for submitting your first pull request! You are awesome! 🤗 |
@nuvious Thank you so much for opening this issue and pull request! It is possible, using commands like
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. |
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. |
Any updates @nuvious? |
Hey @krassowski, looking at it again today between work and school stuff. Got the development environment up today and did some manual tests to evaluate if the fix is working. Going to try to get into the test suite stuff today; not sure why my changes would result in the above failures, but I can dig a little deeper as I familiarize myself with the test pipelines. |
Didn't need to do this, can just merge to main in my own branch to kick of testing CICD
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 #16254User-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