-
Notifications
You must be signed in to change notification settings - Fork 216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nav() now displays some debugging information on KeyError #540
Conversation
There's a user reporting a KeyError in nav(), so I've changed it to print out the key and dict for future debugging. Also, a bare Exception was used to capture a dictionary failure, removed that since my change checks for the existance of the key in the dictionary. If this is objectionable at all, you probably should change it to catching a KeyError rather than catching Exception.
Did you test this change?
Simply running the |
This goes back to catching exceptions because the `root` can also be a list during the loop lifetime. Narrowed down to KeyError and IndexError exceptions, enrich the exception msg with details about the failure.
To make for an easier review, I've restructured the nav() change to more closely match the original.
I couldn't originally get the tests working, I believe this change addresses that problem. |
Sorry, it still doesn't work.
|
Missed a failing test, having to catch TypeError as well. Also realized that I need the try/catch in the loop since it uses the value of "k" in the information message.
Sorry for the delay in getting back to this, busy end of the week. I missed that exception, thanks for pointing it out. I think this modification resolves that issue. One issue I'm running into that perhaps you can help with is that I'm getting a bunch of config-related test failures, and I didn't see that None error among those other ones. The test instructions say to run: |
Sorry about the |
What's the advantage of moving the try-except inside the loop? Isn't that even more overhead? |
Having it in the loop means it has access to the value of "k" to be able to log. It could be moved outside the loop but would lose that data, but might still provide enough information to track down, given more effort. |
But it's bad: https://docs.astral.sh/ruff/rules/try-except-in-loop/ And with |
LSP is telling me that "k is unbound" if I move the try/except out of the loop, but if I run a test I am getting the loop variable past the loop: try:
for x in [1,2,3,4]:
if x == 3:
1 / 0
except Exception as e:
print(f"Failed, {x}") Results in: "Failed, 3". So, I've moved it out of the loop. |
There's a user reporting a KeyError in nav(), so I've changed it to print out the key and dict for future debugging. Also, a bare Exception was used to capture a dictionary failure, removed that since my change checks for the existence of the key in the dictionary. If this is objectionable at all, you probably should change it to catching a KeyError rather than catching Exception.
This is to gather information for #539