Improve error handling when background services are not initialized #840
Conversation
Often during the early stages of development, people might just try to run pytest, and not pylint every time. This makes it hard to identify when they forget to call `super().__init__()` when deriving from `BackgroundService`. This commit improves error handling in `BackgroundService` to provide a more meaningful error and a solution to an otherwise hard to debug issue. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
github-actions
bot
added
the
part:actor
BeforeAfter |
|
Isn't the missing call to |
did you read the description? |
Haha, yes, but I guess I missed the first line, sorry. I'm not sure it is worth adding the runtime cost of checking if it was initialized everywhere just to get a better error message in early stages of development. I would like more opinions on this one, @matthias-wende-frequenz @Marenz @daniel-zullo-frequenz ? |
llucax
requested review from
Marenz,
daniel-zullo-frequenz and
matthias-wende-frequenz
it is an assert that doesn't allocate and runs only when a background service is starting up or stopping, so it is not really a runtime cost. but it makes development easy especially for external developers who might not always want to start with repo config, etc, but might just want to start coding, and setup the tooling later, because that's what we've been showing in the demos as well. In such a scenario, it is just annoying, and people would be more inclined to think the SDK is broken because the error message is totally unhelpful. |
I mean that's not exactly correct, looking at the diff it's also running on |
|
Yeah, it is true that runtime cost shouldn't be an issue, and now that I think about it, that's not the real/main reason I'm concerned about this PR. I can understand it might have been annoying to bump to that weird error once, but for me it feels a bit weird to add asserts to check for the same we are checking with linters, just because maybe one forgets to run the linter one time, it is also about polluting the code, specially if we start doing this more and more. I mean, I'd be fine to merge this one if it was really hard to figure out what was going on and would have save you some time, but I don't think it makes sense as a general practice. |
|
In any case, this will probably go away when we fix #826, so I'm fine with merging it. I will still give it a bit more time before the approval to see if @matthias-wende-frequenz or @daniel-zullo-frequenz also have any opinion on the topic. |
I think you're only considering frequenz users who would all use repo-config, etc. It is not impossible to imagine external users of the sdk that don't have pylint as part of their workflow. |
|
I agree the current patch is fine for this particular scenario but it shouldn't be considered as a good/general practice, otherwise all the attributes of the parent class would need to be checked as well. |
a lot of people just use flake8, and that doesn't detect this issue. And pyright (the LSP server I think most people use with editors) doesn't detect them either. |
|
hmm Pylint detected it in VS code if not calling
|
|
Oh wow, if vscode does, then I'm sure other popular things like pycharm do as well. Ok, maybe this was just a side effect of not using the popular tool. Sorry about the noise. :-) |
|
Just for the records,
|
|
interesting, I guess my pyright is just badly configured then. |
4 participants
Often during the early stages of development, people might just try to run pytest, and not pylint every time.
This makes it hard to identify when they forget to call
super().__init__()when deriving fromBackgroundService.This PR improves error handling in
BackgroundServiceto provide a more meaningful error and a solution to an otherwise hard to debug issue.