Fix/empty code fatal #26
Conversation
- Return an AST for the module for empty files - Dont lose the AST on errors that produced it anyway - Updated tests, improved typing
| elif ast: | ||
| ret_ast = ast | ||
| else: | ||
| # Empty modules are still modules |
There was a problem hiding this comment.
I'm not sure if this function returns only errors or not. In the former case I'd say it's better to return an empty AST (or at least we should decide what to expect in that case and document it), in the latter not sure if this fallback case should be handled in this method (since its name suggests that it's for returning errors). Maybe the code is correct but the name should be changed.
There was a problem hiding this comment.
The function produces and error result and sends it; it doesn't return anything as function return value. Tried returning the empty AST before in the case of the empty module but unfortunately the SDK at some point turns and empty AST into a fatal error which is actually as defined (fatal error is the lack of an AST) but undesirable in this case.
Actually, now that I think again about it, empty code shouldn't be an error - but it will still produce this "module only" AST since in Python eve binary modules can have meaning (like __init__.py in this case). So I'll change that part.
| @@ -30,6 +30,14 @@ class RequestCheckException(Exception): | |||
| """ | |||
| pass | |||
|
|
|||
| class EmptyCodeException(Exception): | |||
There was a problem hiding this comment.
It seems that this exception replaces the RequestCheckException one (I've not seen it used at least). If that's the case, remove the old one
There was a problem hiding this comment.
It replaced it. Removing the old one.
|
🎉 Would be super-happy to test it locally with https://gist.github.com/bzz/c0c3dbcab5fecbe48e22167e2ad78595 as soon as it's ready. |
- Removed unneeded code
|
Why has this been merged while having requested changes? |
|
Sorry @abeaumont I missed your last message. All your feedback on both points was translated into changes as you can see. Did I miss something? |
|
@juanjux I didn't look at it since it was already merged, but looking at it now I see that |
|
@abeaumont true, I'll remove it on a separate PR I'll do as soon as I've five minutes free to remove the MsgPack support, thanks. |
Fixes #25 which could be related with SDK/#34.