openaustralia / morph

Take the hassle out of web scraping
https://morph.io
GNU Affero General Public License v3.0
461 stars 74 forks source link

SQL queries should use double quotes, we currently suggest single quotes #1144

Closed equivalentideas closed 7 years ago

equivalentideas commented 7 years ago

This has been extracted from https://github.com/openaustralia/morph/pull/1128

SQLite prefers double quotes around table and column names. Use of single quotes in select_first_ten() here causes the default example query in API docs to fail if a user edits it to add (eg) an ORDER BY if that user also uses single quotes, as the example encourages.

...

I double-checked this and found it's correct - double quotes should be used for identifiers like table and column names 👍

...

Let's be clear: I'm proposing this solely because it's a moderately-serious UI fail - where this query is reprinted in the API documentation it encourages users to use single quotes, the first thing I tried was to add an ORDER BY to the example, and with single quotes that query outright fails to work.

So yeah, by all means, but this isn't rooted in me being anal about SQL style, but about whether the API works as documented. Right now, it (slightly) does not.

Here's an example of this in the interface here https://morph.io/documentation/api :

screen shot 2017-04-06 at 5 23 06 pm

handelaar commented 7 years ago

Specifically, the documentation error results from echoing the string output of a function that creates a query containing single quotes. Apparently it is important that a fix should not escape a double-quote mark, but must also perform both of these unrelated tasks.

Making the query "correct" was not the goal of the first attempt at fixing this. Fixing the documentation was. With single quotes, as suggested by the docs page, almost no more-complex queries will function as expected; you'll get either the wrong results, or an error.

handelaar commented 7 years ago

👏