BlitzGateway.connect raise on error

[manics]

Adds a flag so that BlitzGateway.connect will always propagate an exception if thrown. The default behaviour remains unchanged: sometimes it'll raise, sometimes it won't.

[mtbc]

The code changes certainly look plausible and helpful.

[joshmoore]

This has not been included in merge-ci:

  - PR 207 manics 'BlitzGateway.connect raise on error' (status: pending)

Re-opening to see if it goes green. A few thoughts on the API though:

• pretty sure we're still in camel case land, but I wonder if PEP8 style might make sense: raise_on_error. Another option would be strict.
• I wonder if there is room for having this set on the base object so all subsequent calls to connect() use the raiseOnError state.
• I also wonder if anyone has an idea on eventually making this the default.

[will-moore]

Code looks good. I can test locally...

[will-moore]

Looks good (and useful).

>>> conn = BlitzGateway('will', 'pw', host='localhost', port=4064)
>>> conn.connect()
False
>>> conn.connect(raiseOnError=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/wmoore/Desktop/PY/omero-py/target/omero/gateway/__init__.py", line 2270, in connect
    self._createSession()
  File "/Users/wmoore/Desktop/PY/omero-py/target/omero/gateway/__init__.py", line 2086, in _createSession
    self._ic_props[omero.constants.PASSWORD])
  File "/Users/wmoore/Desktop/PY/omero-py/target/omero/clients.py", line 653, in createSession
    prx = rtr.createSession(username, password, ctx)
  File "/Users/wmoore/opt/miniconda3/envs/omeroweb/lib/python3.6/site-packages/Glacier2_Router_ice.py", line 258, in createSession
    return _M_Glacier2.Router._op_createSession.invoke(self, ((userId, password), _ctx))
Glacier2.PermissionDeniedException: exception ::Glacier2::PermissionDeniedException
{
    reason = Password check failed for 'will': [id=2]
}

I would like this to become the default (when we're ready to release a breaking change).
In fact, it's almost not a breaking change since you'd likely get an exception when you try to use conn if the connect failed. Then you'd just get a different exception.
If the default raiseOnError is True, then you probably wouldn't need to add it to the base object since I can't see why you'd want connect() to fail silently when it's used within other methods, since the methods themselves would then raise a different exception anyway.

[joshmoore]

@manics : any comments on any of the API comments here?

[manics]

I agree with @will-moore but isn't that still a breaking change which would have to wait for 6.0.0?

[joshmoore]

And anything from #207 (comment)?

In general, this is proposing adding API to an already pretty complicated class. It's worth brainstorming and getting the design, since it'll likely be around for a while.

[manics]

All seem reasonable, I don't have a preference. Anyone else? Alternatively if 6.0.0 is not too far off we could go for the breaking change, maybe even not have a parameter at all, i.e. always throw and the caller can catch the exception if required?

[manics]

Any more thoughts on what to do here? I agree the BlitzGateway is already complicated, but this change makes it easier to use. Currently if someone asks how to figure out a BlitzGateway.connect() failure we have to tell them to use omero.client so that we can see the error.

Is there a definite plan to refactor the BlitzGateway with the existing API, as opposed to writing a new gateway?

[will-moore]

I vote for changing the default behaviour to raiseOnError=True(breaking change) on the next major release.

[joshmoore]

Any more thoughts on what to do here?

I'd like to see a few more API options to choose from on style based on the various points I've added.

Is there a definite plan to refactor the BlitzGateway with the existing API,

My primary next wish-list-item would be to split gateway into submodules and then re-import everything back into the top-level namespace. Other than that I think a rewrite has only been mentioned "if necessary"

Otherwise, 👍 for the new behavior.