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

Preference for quoting identifier mechanism #1436

Merged
merged 6 commits into from Jun 30, 2018

Conversation

Projects
None yet
3 participants
@mgrojo
Copy link
Contributor

mgrojo commented Jun 18, 2018

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 #1280.
Preference for quoting identifier mechanism
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.

mgrojo added some commits Jun 18, 2018

Revert to grave accents as default quoting
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
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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Jun 18, 2018

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.

@justinclift justinclift referenced this pull request Jun 21, 2018

Closed

Deprecated quotes used in generated SQL #1440

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

This comment has been minimized.

@MKleusberg

MKleusberg Jun 21, 2018

Member

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?

@@ -11,9 +11,26 @@ namespace sqlb {

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

escapeQuoting customQuoting = GraveAccents;

This comment has been minimized.

@MKleusberg

MKleusberg Jun 21, 2018

Member

This should be made static.

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Jun 21, 2018

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.

  1. 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...

  2. 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.

  3. 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.

  4. 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

This comment has been minimized.

Copy link
Member

MKleusberg commented Jun 21, 2018

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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Jun 21, 2018

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 😃

imagen

imagen

imagen

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
imagen
😮 😄

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jun 21, 2018

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. 😄

mgrojo added some commits Jun 23, 2018

New tests for the new standard quoting and for the other quoting styles
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.
Pref: "SQL editor font" and its size on the same row and other improv…
…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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Jun 23, 2018

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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Jun 30, 2018

@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.

@mgrojo mgrojo merged commit 719416e into master Jun 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Jul 10, 2018

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

@MKleusberg MKleusberg deleted the identifier_quotes branch Jul 10, 2018

MKleusberg added a commit that referenced this pull request Aug 12, 2018

Fix editing table with custom display formats set
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment