Also raise exceptions from FakeSession.

[stephen-soltesz]

SimpleSession.get and walk now raise exceptions when no results were returned. So, FakeSession should behave the same way.

This PR rearranges some previously private functions to be used by both SimpleSession and FakeSession.

---

This change is

[coveralls]

Coverage increased (+0.008%) to 98.641% when pulling 1c7b988 on stephen-soltesz:safe-varbind into cea8b16 on m-lab:master.

[mtlynch]

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.

---

site-packages/mlab/disco/simple_session.py, line 98 [r1] (raw file):
I feel like these changes make both classes significantly more difficult to understand. FakeSession in particular had both implementations as one-liners (a very nice thing), whereas now the reader has to read 20+ lines to understand the implementation.

I'm also uncomfortable with sacrificing clarity in the SimpleSession to facilitate code reuse in FakeSession. I feel like we're sacrificing clarity by requiring _convert_result to take a function as a parameter.

Could we keep the implementations independent and change `FakeSession's implementation to something like this?

def get(self, oid):
  return self._find_oid(oid)

def _find_oid(self, oid):
  try:
    return self._mib[oid]
  except KeyError:
    raise SNMPError('FakeSession has no value for OID: %s' % oid)

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.01%) to 98.643% when pulling 8ab5a19 on stephen-soltesz:safe-varbind into cea8b16 on m-lab:master.

[stephen-soltesz]

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.

---

site-packages/mlab/disco/simple_session.py, line 98 [r1] (raw file):
Okay. I don't want to make the classes less clear or harder to understand. I've added the _find_oid method to FakeSession to keep the implementations separate and removed the "convert" parameter from _convert_result.

I think this better preserves the original complexity of both classes, while moving some logic that was previously unnecessarily part of SimpleSession to the module level.

How does it look?

---

Comments from Reviewable

[mtlynch]

---

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

site-packages/mlab/disco/simple_session.py, line 103 [r2] (raw file):
Is there a requirement that the error message be exactly the same in both cases? It seems strange that we'd say it's an unknown error when really we control everything about FakeSession and we know why there's no result: the caller never prepared one.

---

site-packages/mlab/disco/simple_session.py, line 121 [r2] (raw file):
Can we expand the docstring a bit? Now that it's a little farther away from the related functions, it's harder to keep track of what we're converting to what. I think it'd be helpful to have the args explicitly documented.

---

Comments from Reviewable

[stephen-soltesz]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

site-packages/mlab/disco/simple_session.py, line 103 [r2] (raw file):
There's no requirement that they be the same. You're right that the caller should know what to expect from a FakeSession.

Changed the error message to simply "Empty result set."

---

site-packages/mlab/disco/simple_session.py, line 121 [r2] (raw file):
No problem.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.01%) to 98.643% when pulling 28f424d on stephen-soltesz:safe-varbind into cea8b16 on m-lab:master.