-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
lp1890116: Fix import of external playlists #3017
lp1890116: Fix import of external playlists #3017
Conversation
Could someone please review this PR? It just reverts to the previous, string-formatting approach with some minor code improvements.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you.
QSqlQuery query(m_database); | ||
FieldEscaper f(m_database); | ||
QString queryString = | ||
const auto playlistIdNumber = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think using auto
instead of QString
only saves 2 chars and makes it less apparent which type we're dealing with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguing about individual cases is the wrong approach. The length of 'QString' should not matter, it could be any type. The usage of auto must not depend on how many characters it saves.
More appropriate rule: auto
is allowed if the resulting type is immediately apparent from the right-hand-side expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it reduces redundancy, which is worth more than saving 2 characters ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then I'm fine with this.
https://bugs.launchpad.net/mixxx/+bug/1890116
Broken by 085af01
Affects all external playlist views, not only iTunes!
bindValue()
does not seem to work forCREATE TEMPORARY VIEW
.