openlp-core team mailing list archive

[Raoul Snyman <raoul.snyman@xxxxxxxxxxxxxxxxxxxxxxxx>]

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.