[FIX] No panic on an old open request for __init__#539
[FIX] No panic on an old open request for __init__#539
Conversation
Before this was causing panic, while it is safe to just ignore it This PR, makes it panic only on unreachable cases, otherwise, silently leave. It is safe to not process a deleted `__init__,py`
| // Another notification will come for the deletion of the file, so we just warn here. | ||
| warn_or_panic!("Trying to create a custom entrypoint on a namespace symbol: {:?}", new_sym.borrow().paths()); | ||
| if Path::new(file_path).exists() || !file_path.ends_with("__init__.py") { | ||
| // a file exists, or does not end with __init__.py |
There was a problem hiding this comment.
are you sure that !file_path.ends_with("__init__.py") is the right test?
It's confusing me. If we are on a namespace symbol, I thought you would want to test if an __init__.py file exists or not in the namespace. Something like:
Path::new(file_path.join("__init__.py").exists()
I don't see the point of Path::new(file_path).exists() either
There was a problem hiding this comment.
So the goal of this condition is not panic when we have an __init__.py file that does not exist anymore.
This is the case that we can predict, and is safe to skip, other cases we should still warn or panic, which in theory should be unreachable.
I was able to replicate the panic, by having a folder ( custom entry points) having __init__ and __manifest__ and then trigering a rebuild by editing a file in the orm, while it is reloading, I delete __init__ and then doing a hover in __manifest__
Then when create_from_path gets called during the rebuild, (due to the py package being in self import), then since we check if __init__.py exists or not, we get false, so we create a namespace which is wrong.
But it should be okay, because there should come a didClose and/or didDelete and then we will delete the symbol.
So that is why we want to only catch the case that there is a file that exists and is not __Init__ then something else is wrong and we want to catch it.
Another thing to note is, probably #541 already will prevent the panic from happenign at all, because usually the symbol will be removed correctly, while it was not before, but since I am not sure about all the cases, it might still be safe to keep the change of the check here?
|
This code should be in unreachable code. If we reach it, we should not hide the panic, as it's an error |
2 participants
Before this was causing panic, while it is safe to just ignore it
This PR, makes it panic only on unreachable cases, otherwise, silently leave. It is safe to not process a deleted
__init__,py