Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Updated Time Range Parsing in Report Details #195

Merged

Conversation

davidcui1225
Copy link
Contributor

Issue #, if available:
N/A
Description of changes:
Time range in Report Details for Visualizations and Saved searches from in-context menu would show as undefined. Updated the parsing so it displays correctly.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left question regarding regex

let timeStringRegex = /time:\(from:.*,to:.*\)/;

let timeStringRegexArray = timeStringRegex.exec(queryUrl);
let timeString = timeStringRegexArray[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a note, you can probably do const [timeString, fromDateString, toDateString] = queryUrl.match(/time:\(from:(.+),to:(.+?)\)/) might be easier

also I think the lazy match in to:.*?\) is necessary? wouldn't to:.*\) match to the last )?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - will make change

Copy link
Member

@zhongnansu zhongnansu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address the comment before merge

@davidcui1225 davidcui1225 merged commit 66e4cc8 into opendistro-for-elasticsearch:dev Nov 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants