Preference for quoting identifier mechanism #1436

mgrojo · 2018-06-18T16:29:56Z

A new option is added to the SQL tab in Preferences for choosing which
quoting characters must be used by the application when it inserts an
identifier in SQL code. The three options supported by SQLite are
available.

Default quoting characters have been changed from `Grave accents` to
"Double quotes" (SQL standard).

This options also affect the highlighting of double quoted strings: when
the SQL standard is selected, double quoted expressions are highlighted as
identifiers, otherwise as literal strings.

See issues #683 and #1280.

Matters to be discussed:

Default: should we stay by the custom and use `grave accents` or follow the standard and use "double quotes"? I prefer standard, but the highlighting provided by QScintilla isn't reversible: if we choose to use double quotes for identifiers, the quoted table identifiers loose their specific style. We can get consistent behaviour if we use QsciLexerSQL::setQuotedIdentifiers but only by loosing the table style in all cases, with the positive side of getting proper highlighting of keywords inside `grave accents`. Note that unquoted identifiers written by the user would still, in any case, be highlighted as tables.
Since escapeIdentifier(id) doesn't belong to a class, the usual reloadSettings slot cannot be used. Please, validate the proposed solution.
In order to not clutter the Preferences window I tried to move SQL editor font size box to the same row as SQL editor font but I was unable to do it with qtcreator and didn't dare to change it manually without knowing if the UI change would be welcome.
Should a None option be available? This would left the responsibility of using appropriate identifiers to the user. I've discarded this option because it feels problematic, but it was suggested in Automatic quoting identifiers inconsistent and probably not desirable. #1280.

A new option is added to the SQL tab in Preferences for choosing which quoting characters must be used by the application when it inserts an identifier in SQL code. The three options supported by SQLite are available. Default quoting characters have been changed from `Grave accents` to "Double quotes" (SQL standard). This options also affect the highlighting of double quoted strings: when the SQL standard is selected, double quoted expressions are highlighted as identifiers, otherwise as literal strings.

The former quoting is expected by the tests, so the former behaviour is restored as default, until a decision about the best default value is made.

mgrojo · 2018-06-18T17:45:35Z

I changed the default value to `grave accents` because it was breaking the tests. I didn't know how to run the tests until know. I'll run them before any commit.

When we decide which is the best value for the default, I'll update the tests.

MKleusberg · 2018-06-21T08:40:47Z

src/sqlitetypes.cpp

+        // so we rely on the user to not enter these characters when this kind of quoting is
+        // selected.
+        return '[' + id + ']';
+    }


This produces a control reaches end of non-void function warning for me even though you have covered all possibilities in that switch statement. Maybe just add a default branch which falls through to DoubleQuotes?

MKleusberg · 2018-06-21T08:58:28Z

src/sqlitetypes.cpp


 QStringList Field::Datatypes = QStringList() << "INTEGER" << "TEXT" << "BLOB" << "REAL" << "NUMERIC";

+escapeQuoting customQuoting = GraveAccents;


This should be made static.

MKleusberg · 2018-06-21T09:42:02Z

Thanks for taking care of this! I wanted to do this a long time ago but never got around it 😄

I have already pointed out two minor things in the code but here is a more in-depth answer for each of your four points.

I would change the default to double quotes. I'm not 100% sure though if I fully understand your worries here. I see what you mean that "tablename" doesn't get highlighted in any way. But I don't see the positive side of setQuotedIdentifiers about highlighting keywords in grave accents because on the one hand it doesn't seem to do that for me and on the other hand I'm not sure it would be correct to highlight keywords inside accents.
Either way, what I'm most confused about is why using the string style works fine for double quotes but using the identifier style doesn't...
I'm fine with the global variable. As noted in the code it should be made static but other than that I don't see a better way to do this.
I just pushed a commit for that. It's not perfect but it does put the two settings in one row. But feel free to remove it again or improve it if you don't like how it looks now. The main problem here is that we're using a QFormLayout which always has exactly two columns. I'm now putting a layout inside the second column. That works but it will never get the spacings between label and spinbox in sync.
I'd say no. Because the quotes are not only used for export etc. but also for internal use, we can't really offer this as an option which will break things.

MKleusberg · 2018-06-21T10:59:31Z

One extra thing here: when you change the default to double quotes and you update the tests, maybe add a few tests for the different settings - just to make sure we cover not only the default case.

mgrojo · 2018-06-21T20:21:45Z

I would change the default to double quotes. I'm not 100% sure though if I fully understand your worries here.

Let me explain it with images 😃

My conclusion is: we should set setQuotedIdentifiers to true, at least when the quoting characters are `grave accents`, because:

it is symmetric to having set the quoting to "double quotes" and
table identifiers containing keywords wouldn't have that mixed style any more, [1]

even though we loose the table style for quoted table identifiers.

[1] Extreme case example: from https://dbhub.io/nicva/State%20of%20the%20Sector%20-%20Volunteers%20Chapter.sqlite

😮 😄

justinclift · 2018-06-21T20:48:15Z

Heh Heh Heh. I wondered if anyone would notice the dbhub.io server is available again.

It's still not GDPR compliant, but I've pinged Cristian Driga again to pick it back up, so we can start working on that too. 😄

Modified some tests for taking into account the new standard quoting and added some more for testing the quoting configuration and for correct parsing of different quoting styles. Default branch in escapeIdentifier() for trying to avoid warning.

…ements "SQL editor font" and "SQL editor font size" are located at the same row for saving vertical space in the Preferences dialog. Accelerator keys for "Wrap lines" and "Quotes for identifiers" have been added. Some buddies have been fixed.

mgrojo · 2018-06-23T17:26:07Z

I consider the feature now complete and it's ready for review.

I've set the default to "double quotes",
added and updated some tests,
finally added setQuotedIdentifiers to true when `grave accents` is selected (I forgot to mention that in the commit log),
added default branch to avoid warning (there was no warning for me, so just guessing),
minor adjustments to the preferences dialog

mgrojo · 2018-06-30T16:14:23Z

@MKleusberg I'll alleviate a bit your review queue by merging this 😃 I think the important concerns are already discussed and addressed. Anyway, don't mind making a review after the merge and comment whatever you would prefer changed and I'll address it then.

MKleusberg · 2018-07-10T18:17:11Z

Thanks a lot, @mgrojo 😄 Looking at your pictures I now understand what you meant and agree with your analysis.

Since PR #1436 we allow configuration of the identifier quotes instead of always using backticks. But in the code for detecting a custom display format on a column the assumption still was that normal columns without a custom display format are always surrounded in backticks. The result of this was that even if there is only a single display format configured for a table, no field of no column can be edited anymore. This commit restores the original state which would only disable editing for the columns with a custom display format.

mgrojo added enhancement Feature requests. SQL labels Jun 18, 2018

mgrojo added 2 commits June 18, 2018 19:03

Revert to grave accents as default quoting

265b077

The former quoting is expected by the tests, so the former behaviour is restored as default, until a decision about the best default value is made.

Revert to grave accents as default quoting

da8057c

The former quoting is expected by the tests, so the former behaviour is restored as default, until a decision about the best default value is made.

justinclift mentioned this pull request Jun 21, 2018

Deprecated quotes used in generated SQL #1440

Closed

7 tasks

Move font size settings into the same row in settings dialog

9e0e330

MKleusberg reviewed Jun 21, 2018

View reviewed changes

mgrojo added 2 commits June 23, 2018 17:57

mgrojo merged commit 719416e into master Jun 30, 2018

MKleusberg deleted the identifier_quotes branch July 10, 2018 18:17

mgrojo restored the identifier_quotes branch October 14, 2023 21:27

mgrojo deleted the identifier_quotes branch October 14, 2023 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Preference for quoting identifier mechanism #1436

Preference for quoting identifier mechanism #1436

Uh oh!

mgrojo commented Jun 18, 2018

Uh oh!

mgrojo commented Jun 18, 2018

Uh oh!

MKleusberg Jun 21, 2018

Uh oh!

MKleusberg Jun 21, 2018

Uh oh!

MKleusberg commented Jun 21, 2018

Uh oh!

MKleusberg commented Jun 21, 2018

Uh oh!

mgrojo commented Jun 21, 2018

Uh oh!

justinclift commented Jun 21, 2018 •

edited

Loading

Uh oh!

mgrojo commented Jun 23, 2018

Uh oh!

mgrojo commented Jun 30, 2018

Uh oh!

MKleusberg commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		QStringList Field::Datatypes = QStringList() << "INTEGER" << "TEXT" << "BLOB" << "REAL" << "NUMERIC";

		escapeQuoting customQuoting = GraveAccents;

Uh oh!

Preference for quoting identifier mechanism #1436

Preference for quoting identifier mechanism #1436

Uh oh!

Conversation

mgrojo commented Jun 18, 2018

Uh oh!

mgrojo commented Jun 18, 2018

Uh oh!

MKleusberg Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

MKleusberg Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

MKleusberg commented Jun 21, 2018

Uh oh!

MKleusberg commented Jun 21, 2018

Uh oh!

mgrojo commented Jun 21, 2018

Uh oh!

justinclift commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgrojo commented Jun 23, 2018

Uh oh!

mgrojo commented Jun 30, 2018

Uh oh!

MKleusberg commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinclift commented Jun 21, 2018 •

edited

Loading