Skip to content

Add additional error handling for livery parsing. #364

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.

Sign up for GitHub

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

Open
SnappyComebacks wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add additional error handling for livery parsing. #364

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions dcs/liveries/livery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,16 @@ def from_lua(code: str, path: str) -> Optional[Livery]:
logging.exception("Could not parse livery definition at %s", path)
return None
livery_name = data.get("name", path_id)

countries_table = data.get("countries")
if countries_table is None:
countries = None
else:
countries = set(countries_table.values())
order = data.get("order", 0)
countries = None
Copy link
Collaborator

@DanAlbert DanAlbert Feb 9, 2024

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.

Copy link
Contributor Author

@SnappyComebacks SnappyComebacks Feb 9, 2024 •
edited
Loading

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.

Copy link
Collaborator

@DanAlbert DanAlbert Feb 9, 2024

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.

Copy link
Collaborator

@DanAlbert DanAlbert Feb 9, 2024

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.

Copy link
Collaborator

@DanAlbert DanAlbert Feb 9, 2024

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.

if countries_table is not None:
try:
countries = set(countries_table.values())
except AttributeError:
logging.exception("Could not parse 'countries' field for livery at %s", path)

order = data.get("order", 0)
order = None if path_id == "default" else order
if order is not None and not isinstance(order, int):
try:
Expand All @@ -104,11 +107,15 @@ def from_lua(code: str, path: str) -> Optional[Livery]:

@staticmethod
def from_path(path: str) -> Optional[Livery]:
if os.path.isdir(path):
return Livery.from_directory(path)
elif path.endswith(".zip"):
return Livery.from_zip(path)
return None
try:
if os.path.isdir(path):
return Livery.from_directory(path)
elif path.endswith(".zip"):
return Livery.from_zip(path)
return None
except Exception:
logging.exception("Could not parse the livery at %s", path)
return None

@staticmethod
def from_directory(path: str) -> Optional[Livery]:
Expand Down