Disable SQL auto-complete by default

[jezdez]

To quote @harterrt in 1477317:

The SQL formatter in re:dash should not be enabled by default. It disagrees with the SQL style guide and is generally more harmful than helpful in my experience.

[fbertsch]

Alternatively, can we fix the formatter to match our style guide?

[harterrt]

I would still argue to remove the autosuggest functionality. For example typing SELECT<enter> gets autocompleted to select

[jezdez]

FTR we use the sqlparse library to do the reformatting, if you can match the style to the provided options, sure it's possible to match it. Otherwise this would need to be replaced with something else.

I should note, this feature is planned to be ported upstream, so any solution would require a non-NIH approach.

[fbertsch]

Should we split the concern of the auto-complete and it's relation to SQL formatting into a separate issue?

[tdsmith]

It looks like autocomplete is provided by Ace and it reuses the syntax highlighting rules in https://github.com/ajaxorg/ace/blob/master/lib/ace/mode/sql_highlight_rules.js#L39-L43 to suggest keywords.

[harterrt]

I'm fine with splitting this issue but my primary concern is with the autosuggest functionality. Sorry for the unclear description (and grumpy tone).

The SQL formatter only triggers when clicked so it's not an issue for me. The autocomplete is on by default and does interfere with my workflow. As noted above, the autocomplete appears to prefer lower-cased keywords and makes distractions suggestions even when typing variable names.

For example, in this query I'm trying to filter on submission_date but after typing sub I get a suggestion to autocomplete to the function days_ago.

It would be better if this functionality was off by default.

[harterrt]

@jezdez What do you think about turning the auto-complete off by default?

[harterrt]

Friendly ping.

[jezdez]

@harterrt Whoops, missed this last week! 👐

Yes, let's turn auto-complete off by default.

Also I was wrong in my earlier comment and @tdsmith was correct, I simply confused formatting and auto-completion. Sorry!

[jezdez]

I've updated the title to better match what @harterrt wants.

BTW, I should note, since the default for the auto-complete is something that is set in the upstream repo (despite us having the additional fork feature of the autocomplete toggle) we'll have to consult with upstream to solve this by introducing a config value there to fully get this done.

[harterrt]

Hey all, just checking in on this. Do we have an ETA for when this may be turned off by default?

Thanks!

[jezdez]

Hey @harterrt, no estimate since it'll require changes upstream and we've been working on a related change in getredash#2746.

If you think this is urgent, we can make an exception and add the feature in our fork, but since we're trying to move changes in our custom fork upstream it would make sense to do that only if it's a blocker for your workflow. I hadn't read your issue so far with that scope, apologies in case that happened.

[jezdez]

@alison985 Do you think you could add this feature to getredash#2746?

[harterrt]

Thanks, @jezdez. I agree we should solve this upstream. It's an annoyance, but not worth maintaining it in the fork. I only pinged this thread to make sure the issue doesn't slip through the cracks.

When should I check back next? 4 weeks?

[jezdez]

@harterrt Understood, thanks for understanding. 4 weeks sound like a good timebox.

[alison985]

We closed getredash#2746 because the changes aren't relevant upstream yet. I've opened getredash#2893 upstream in order to have any needed conversation about adding a configuration toggle for this setting.

[alison985]

@jezdez Arik talks about local storage in getredash#2893 - Does that mean a cookie? Can you point me to where local storage is talked to in the code?

[alison985]

@jezdez Bumping up this question.

[jezdez]

@alison985 I think he refers to the web storage backend window.localStorage which is a web platform technology key/value-store available in all major browsers.

[alison985]

@washort I think I'm running into a React difference. What's the React equivalent of $watch that we're using? A quick google shows multiple npm modules, and our package.json doesn't list one.

[rafrombrc]

Well, we have good news and bad news about this ticket. The bad news is that this particular effort to fix up auto-complete is being scrapped. The good news is that this effort has prompted the following decisions:

• Whether or not autocomplete is turned on will become a user preference that will be available on a per-user basis shortly after Add "Active at" column to user list. getredash/redash#3026 is merged (relevant discussion in Improve Autocomplete toggle getredash/redash#2984).

• Autocomplete will be getting a ground up overhaul in the upstream redash soon, tracked in Autocomplete V2 getredash/redash#3067.

• Upstream will soon merge one of our band-aid fixes to autocomplete that helps minimize the performance issues: Always use basic autocomplete. getredash/redash#3079.

[alison985]

@rafrombrc @jezdez For getredash#3079 I'm not sure we want that in the codebase after Janni's comments here when I tried to port it before.