Skip to content

Add highlights sql output#2790

Merged
chrisknoll merged 5 commits intomasterfrom
add_highlights_sql_output
Dec 26, 2022
Merged

Add highlights sql output#2790
chrisknoll merged 5 commits intomasterfrom
add_highlights_sql_output

Conversation

@anton-abushkevich
Copy link
Contributor

Resolves #2775

  • added SQL syntax highlight
  • added variables (parameters) replacement (switch between parameters names and parameters values)

Copy link
Collaborator

@chrisknoll chrisknoll left a comment

Choose a reason for hiding this comment

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

When the view initially presents, the options text input fields are read only. However, you can click 'replace values' (which will replace the defaults in the SQL output, and then the fields become enabled. I think these text fields should always be enabled (ie: never read-only).

Copy link
Collaborator

@chrisknoll chrisknoll left a comment

Choose a reason for hiding this comment

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

Let's make some changes to behave more like a knockout application where we depend on observables instead of managing our own subscriptions:

  1. Replace ko.compuated() with ko.pureComputed when the observable is just a computed alue (ie: the sqlParams).
  2. Remove subscriptions and instead just store user-entered fields as observables.
  3. sqlParamsList should return a key-value where the key is the param name, and the value is an observable(). This way, changing the value of the input will automatically trigger the refresh of the SQL.
  4. sqlText should be a pureCompuated()) calculated by replacing the sqlText() with the specified paramater values in sqlParamsList (if checkbox is checked) and then render the html with highlightJS().

The flow of the component should be:

  1. The root 'sql' comes is as a component param and is observable (which will update when cohort definition changes or a new cohort definition is loaded)
  2. sqlParams (pureCompted returning an array) depends on sql, which will yield a name-value pairs of paramater names and observables() which store the user input. This pureCompted will be used in the foreach: sqlParams. If the sql canges in some way, this will refresh these params. and re-render the foreach elements.
  3. The input fields can be bound to the value: databinding so that they don't update the observables until the textbox is unfocused (via tabbing off the field).
  4. sqlText() will be a pureComputed() which will generate the HTML to render in the pre element. This can be bound to a pureCompuated which will do the appropraite find/replace and SQL highlighting from sql. This means that if sql, sqlParams, or the observables inside sqlParams change (via the textboxes), this computed will refresh and the <pre> element will be updated.

This should eliminate the need to maintain your own subscriptions, and handle any 'input' events in the event handler.

In addition, is the spread ... operator necessary, can we not just return a 'Set' here:

		sqlParamsList(templateSql) {
			const regexp = /@[-\w]+/g;
			const params = templateSql.match(regexp);
			const paramsList = new Set(params);
			return [...paramsList];
		}

@TitrS
Copy link
Contributor

TitrS commented Dec 15, 2022

@chrisknoll Hello Chris! Thank you for your comment. I changed the logic of component after that. I tried to realize the full flowm but i didn't.
sqlText and sqlParams depend on not only from sql. It depens on from dialect also, we have to change it after async request await cohortService.translateSql so i kept them as observable. Especially thank you for help with subscriptions. It looks better now! Let'me know if i have to change some else or if i misunderstood something.

@chrisknoll chrisknoll merged commit 996b2c6 into master Dec 26, 2022
@delete-merged-branch delete-merged-branch bot deleted the add_highlights_sql_output branch December 26, 2022 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cohort SQL output – UI enhancements

3 participants