Qreator

Merge lp://staging/~stefan-schwarzburg/qreator/qreator_vendor_plugins into lp://staging/qreator

  1. qreator_vendor_plugins
  2. Merge into trunk
Proposed by Schwarzburg
Status: Merged
Merged at revision: 170
Proposed branch: lp://staging/~stefan-schwarzburg/qreator/qreator_vendor_plugins
Merge into: lp://staging/qreator
Diff against target: 91 lines (+30/-4)
4 files modified
qreator/__init__.py (+11/-3)
qreator/qrcodes/QRCodeSoftwareCenterApp.py (+8/-0)
qreator_lib/__init__.py (+1/-1)
qreator_lib/helpers.py (+10/-0)
To merge this branch: bzr merge lp://staging/~stefan-schwarzburg/qreator/qreator_vendor_plugins
Reviewer Review Type Date Requested Status
David Planella Approve
Schwarzburg Needs Information
Review via email: mp+164632@code.staging.launchpad.net

Description of the change

The implementation is a simplified version of my comment on https://trello.com/card/add-vendor-infrastructure/4fc6a3556634e8557016945d/27 :

Each plugin is responsible for checking for vendor compatibility (since we have only one at the moment, this seemed the simplest way and is also in line with the plugin idea). If the vendor compatibilty is not met, the plugin raises an VendorError, which is caught by the importer infrastrucuter.

This has at the moment nothing to do with the colors of the icons.

Also: I was not able to test this on non-ubuntu distros, but they should normally not be named "Ubuntu", and therefore the test wether this is "Ubuntu" should fail...

To post a comment you must log in.
Revision history for this message
David Planella (dpm) wrote :

Nice work as usual, I like the approach.

Comments inline, just minor things:

21 + print "%s disabled due to missing vendor infrastructure." % (name,)

I wonder if we might want to use the logging system and just flag this as a warning message.

43 +

Additional whitespace here, just a nitpick, but it might be worth not introducing it.

67 + pass

Rather than passing here, do we want to perhaps print the warning in the exception as a central place, and let the plugins override the message?

review: Needs Information
lp://staging/~stefan-schwarzburg/qreator/qreator_vendor_plugins updated
170. By Schwarzburg

added the VendorError body which can now hold an error-value; replaced a print statement by a proper logging call.

Revision history for this message
Schwarzburg (stefan-schwarzburg) wrote :

I think changes should adress your comments.

- The logging is implemented this way because the order of plugin-imports and logging setup matters
- The ValueError now has a body with a message. The printing of that message is still left to the structure that catches the error. I think this is more the way it is supposed in python.
- I have removed the whitespace, but added three other :-)
  38: There was one blank line to separate code, so I have added another one at the end of the added lines.
  52-53: After an import line, there should be two blank lines...

review: Needs Resubmitting
Revision history for this message
Schwarzburg (stefan-schwarzburg) :
review: Needs Information
Revision history for this message
David Planella (dpm) wrote :

Merged, thanks!

I made a few small changes before the merge: namely, protecting the USC database module import with a try/exception block and raising a VendorError, as otherwise, the import was executed and it would have raised an ImportError exception if not available in a given platform other than Ubuntu.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: