NF: improve the readability of some strings #17592
Conversation
| "document.getElementsByTagName(\"h1\")[0].style.color=\"" + textColor + "\";" + | ||
| "x=document.getElementsByTagName(\"h2\"); for(i=0;i<x.length;i++){x[i].style.color=\"#E37068\";}" + | ||
| "document.body.style.setProperty(\"background\", \"" + background + "\");", | ||
| """javascript:document.body.style.setProperty("color", "$textColor"); |
There was a problem hiding this comment.
I can open the about page and still see the changes.
Appart from that, it's exactly the same code as before, except that it uses triple-quote and
I'd suggest to use android studio commit tool instead of github one. Because this way you can easily check that everything removed were \ before the ", the " + and + " and everything added are $ before the variables.
The idea is really that we have the same string, but more readable
| @@ -71,16 +71,27 @@ object MetaDB { | |||
|
|
|||
| // Create tables if not exist | |||
| metaDb.execSQL( | |||
| "CREATE TABLE IF NOT EXISTS languages (" + " _id INTEGER PRIMARY KEY AUTOINCREMENT, " + | |||
| "did INTEGER NOT NULL, ord INTEGER, " + "qa INTEGER, " + "language TEXT)", | |||
| """CREATE TABLE IF NOT EXISTS languages ( | |||
There was a problem hiding this comment.
Have all the changes to this file been tested (or are they under coverage)?
There was a problem hiding this comment.
I can at least start a new version of ankidroid on emulator, add and review a card. So it works at least in the sense that if there is a change, it's still compatible with the schema created here. Which seems quite improbable
Specifically, the ones that represents json, sql or js. I found they used to be hard to read, and I expect to lower this issue.
Arthur-Milchior
force-pushed
the
string
branch
from
76d1f27 to
d1edf6d
Compare
There was a problem hiding this comment.
I feel this introduces unnecessary risk: we're changing stable, untested code for no functionality improvements and minor readability improvements with very little testing
I'd be much happier if integration tests were added in future which impact the changed lines, as those both give evidence that the changes are correct, and improve the safety of the codebase
But... it's written, it's probably right, and we're at the start of an alpha cycle, let's get it in
david-allison
added
Needs Author Reply
lukstbit
added
Pending Merge
github-actions
bot
removed
the
Pending Merge
Specifically, the ones that represents json, sql or js. I found they used to be hard to read, and I expect to lower this issue.