Standup rota: Use people-related classes and notify them in Slack #698
Conversation
This will make it easier to read and see the changes in subsequent commits.
This allows access to richer information about people. This enables a later commit in the same PR to use `get_formatted_slack_username`. They're split up to keep the change per-commit small.
A function that has as its only parameter an instance of a certain class is effectively a method of that class, and making that explicit is slightly clearer.
I don't think `Enum` is the best structure for accessing a set of instances.
`Enum` are typically used where you don't care about the actual value, and just
want to restrict the names used. An example given in the Python docs:
```
class Color(Enum):
RED = 1
GREEN = 2
BLUE = 3
```
We do care about the `Person` object referenced. I find the syntax for `Enum` a
bit unintuitive and unreadable too, `People.MIKE` was not `Person('Mike',
...)`, but rather a `Person` object with `name=="MIKE"` and
`value==Person('Mike', ...)`. Enums are more common in languages like
C/C++/Java where they allow the compiler to do microoptimization but I don't
think that's relevant here.
This allows us to make `get_person_from_github_username` a method of
`PeopleDict` rather than a module-level method and `PEOPLE_BY_GITHUB_USERNAME`
constant. It's not possible to do that kind of class-construction-time
calculation as pre-indexing the dict with an Enum.
It also avoids the need for accessing `.value` everywhere, which was less
readable (e.g., was `checker.value.get_formatted_slack_username()` now
`checker.get_formatted_slack_username()`).
This also allows us to write tests of `people.py` that don't rely on the
specific real `People` Enum, but rather test a testing-only `PeopleDict` (next
commit).
This separates testing the classes from the actual team members' data, and makes changing the latter easier without randomly breaking these tests of the general functionality. It also makes it clearer what the tests are actually testing (see next commit which will break them up based on which class they are testing.
…orter`, to avoid it happening without this being noticed.
mikerkelly
changed the title
people class and notify them in Slack
|
One of the advantages of using an enum over a dict is that you get editor autocompletion which is easier and less error prone than using strings for the dictionary keys |
Thanks, Jon, I wasn't aware of that. Sorry for clobbering over without prior discussion. I had the idea as I was approaching a meeting and it seemed more expedient to just get the example out in a commit and discuss in review. Speaking purely personally I find either way about as convenient. I would note that it's also a little inconvenient to have to remember to dereference the value of the enum to access the actual Specific members of the I suppose we need to weigh up the convenience for devs of autocomplete when using particular text editors against avoiding the need to remember to add |
Fixes #695.
This uses the new people-related data structures and allows users to be named by Slack username in the
standup daily mondayrepo, @-ing them in Slack. I tested this in the test Slack workspace (temporarily changing my own slack username locally as the username string is different per workspace apparently).I don't think the
DependabotRotaReporterclass is suitable for this report as that cycles through a list of candidates and reports one of them each week, whereas the standup rota reports 6 people in a certain order each week, with the ordering alternating between "even" and "odd" weeks.Also some changes to the class and test design that are hopefully beneficial, see individual commit messages.