-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add additional error handling for livery parsing. #364
base: master
Are you sure you want to change the base?
Add additional error handling for livery parsing. #364
Conversation
else: | ||
countries = set(countries_table.values()) | ||
order = data.get("order", 0) | ||
countries = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes the wrong data to be exposed. A Livery
with None
countries is available to all countries. We don't know what countries it's valid for. The correct thing to do would be to skip this livery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this if you want, but I had left it with the same behavior as previous. I am surprised to see this change requested, as my interpretation of the prior behavior was it is desirable to allow for an easy way to let a livery be available to all countries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prior behavior is that if the livery does not set the countries field, it is available for all countries. That is DCS's behavior, we have no say in it.
With your change applied, liveries with liveries fields that we failed to parse will be available to all countries in pydcs (because of the except
branch does not return None
). They probably are not available to all countries in DCS, because if that's what the author wanted, they wouldn't have set it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm betting I wasn't clear enough on which part I think the problem is. countries = None
isn't actually the problem here, it's just the beginning of the changed block that has the problem, and the problem is a missing line, which I of course can't comment on :) Adding return None
after logging.exception()
would work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my explanation doesn't match your understanding, lmk which part is wrong. It seems like you and I are 100% agreed on the goal here, one of us has just misunderstood some part of the behavior either before or after.
Based on your comments, I may have misread the PR. I'll take another look tomorrow. |
According to Starfire's bug-report, I guess the livery that's causing the issue is doing something like this: To be honest, I don't remember how DCS will interpret this but it can go two ways:
This is obviously assuming that nothing changed in DCS since I wrote the original code. Also, may I suggest ditching the idea to deserialize the entire livery's description file? The regex approach worked fine, and if the second case is DCS' actual behavior, then that code should work out-of-the-box. Granted, the rest of the refactor was indeed necessary and well executed, but switching up the approach to extract the required data seems to be the root cause of the problem. |
Just verified, the second case applies. I can almost with certainty say that the error is indeed caused by assigning a string to |
Several users of DCS_Liberation had exceptions raised at the call
countries_table.values()
. (See dcs-liberation/dcs_liberation#3231 for context). It is my understanding that this is caused by an installed custom livery that has not been configured by the livery developer correctly.I have added a try/except statements to try to more gracefully handle the error that users have been encountering. In addition, I wrapped the
from_file
code with a try/except to skip a failing livery, log the error, and move on.I do not have any custom liveries downloaded, and users have not provided the failing liveries, so I was unable to test the failure.