buildbot/buildbot — pull request #1066 by jpommerening (2014-02-13 13:47:31+00:00)
Title: Cleanup buildslave.commands(.fs)

Remember #970? Yeah, sorry it took me so long.
I was a little confused as to what to do next.

Let's try to clean up `fs.py` before returning to the other PR.

Your suggestions were
- [x] to remove bare `except`s and only catch expected exceptions
- [ ] log them back to the master?
- [ ] do something about the `assert`s

Regarding **logging the exceptions to the master:** Am I supposed to use `sendStatus` for that? What should I send? I was thinking about doing something like this:
```python
except OSError as e:
log.msg("MakeDirectory %s failed" % dirname, e)
self.sendStatus({'rc': 1, 'header': 'os.makedir raised an exception: %s\n' % str(e)})
```

But that would kill any chance to *quietly* handle these conditions in the BuildStep, right? So I was thinking about using something different than `header` … any suggestions?

Regarding **the assertions:** I skimmed the other slave commands – [`git.py`](https://github.com/jpommerening/buildbot/blob/master/slave/buildslave/commands/git.py#L49), [`shell.py`](https://github.com/jpommerening/buildbot/blob/master/slave/buildslave/commands/shell.py#L24), and all the others – they all just seem to using the `args` dict without checking.

I'm starting to think slave commands should have a consistent way of *declaring* which args are required. Like so:

```python
class Command:
requiredArgs = []

def __init__(self, builder, stepId, args):
# assignments …
missingArgs = filter(lambda arg: not arg in args, self.requiredArgs)
if missingArgs:
raise ValueError('Command %s is missing args: %s' %
(self.__class__.__name__, ', '.join(missingArgs)))
self.setup(args)
```

I'd volunteer to extend the `base.Command` that way and adjust `fs.py`, `shell.py`, `git.py`, `cvs.py` and `transfer.py` accordingly. I not too familiar with the other VCSs, so it'd probably be best for someone else to step up. I could try to defer required args from the source though.

Feedback :question: