-
Notifications
You must be signed in to change notification settings - Fork 68
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
fix parsing timestamp strings #162
base: main
Are you sure you want to change the base?
Conversation
by defautl dayjs parse strings as isoDate. If the string only contain digits, we can assume it is a timestamp and convert it to number giving it to dayjs for parsing.
if (timeString === null) { | ||
return timeString; | ||
} | ||
if (!isNaN(+timeString)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explicit conversion to number here is only to appease typescript who want a number in isNaN, it's not needed in js.
We could add a tsignore comment instead.
Looking more into it, it does conflict with the dayjs case where you give the string '2005' and it return january 1st 2005. 🤔 |
Hey, and thanks for taking a look at this! I believe had something similar in earlier version of the plugin, i.e. a more intelligent time format detection. Unfortunately, some users reported issues like the one you noticed as well, so I decided to simplify it. Instead of detecting the format, I'm thinking maybe we should look into defining the format explicitly. Do you have any thoughts on #165? Specifically, if you think it would solve your use case? |
Yeah it look like the same kind of problem. I feel you could have a "custom time" type that open a modal with the all the knobs to set it up to your needs. I don't know if the grafana ui tools allow this kind of stuff. |
ABegon seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an issue this request fixes, I would love it to be approved.
I look at a code and I doesn't see any issues. It is a good quick fix.
Fixes #161
By defautl dayjs parse strings as isoDate. If the string only contain
digits, we can assume it is a timestamp and convert it to number before
giving it to dayjs for parsing.