Review: Needs Fixing
1227 + # Required attributes
1228 + Class = -10
1229 + Name = -11
1230 + Prefix = -12
1231 + # Optional attributes
1232 + CanDisable = -20
1233 + Availability = -21
1234 + SelectMode = -22
1235 + Filter = -23
1236 + # Optional/custom text values
1237 + ComboBoxText = -30
1238 + DisabledLabelText = -31
1239 + GetFilesTitle = -32
1240 + InvalidSourceMsg = -33
I don't like this, they are not enumerations, they are more like constants. If
they are constants they should rather be in ALL_CAPS at the top of the file,
and used as such. For example:
IMPORTER_CLASS = u'class'
IMPORTER_NAME = u'name'
...
_attributes = {
OpenLyrics: {
IMPORTER_CLASS: OpenLyricsImport,
IMPORTER_NAME: u'OpenLyrics'
}
}
Honestly though, I would just go for this:
_attributes = {
OpenLyrics: {
u'class': OpenLyricsImport,
u'name': u'OpenLyrics'
}
}
Additionally, if that "_attributes" is a class variable (as opposed to an
object attribute), then I'd likely name it "__attributes__" (providing it
doesn't clash with a built-in Python name).
--
https://code.launchpad.net/~sfindlay/openlp/refactor-song-import/+merge/108905
Your team OpenLP Core is subscribed to branch lp:openlp.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
