EDCD / EDDI

Companion application for Elite Dangerous
Other
453 stars 82 forks source link

exception handling for GetSystemExtras #2416

Closed boyestrous closed 2 years ago

boyestrous commented 2 years ago

For the GetSystemData or GetSystemsData functions, if you choose showInformation = false, EDSM will not return a population value for that system. Therefore, when these functions make their calls to the GetSystemExtras, the if statement below will always evaluate false and will not pull Station or Faction data. As it stands, the code just returns your StarSystems without the requested Stations and Factions, even though it is possible to pull Station and Faction data without pulling System Information.

This modification adds Exceptions for this situation. I couldn't find anywhere where these parameters would be set by an end-user, so an Exception seemed the most appropriate.

I wasn't sure this was the best way to handle this potential problem. I thought of a couple of other versions:

1) It appears this IF statement is just there to avoid the extra API calls for empty systems, so technically we could just add to the if statement and allow it to proceed if (starSystem.population > 0 || !showInformation)

2) We could also automatically change the value of showInformation in each of the GetSystemData and GetSystemsData functions. By default, these parameters are all set to true, so if someone intentionally turned off the showInformation setting, I would want them to get a more prominent warning.

if ((showFactions || showStations) & !showSystemInformation)
{
    Logging.Warn("systemInformation must be true if showFactions or showStations is true");
    showSystemInformation = true;
    Logging.Warn($"systemInformation has been automatically changed to {showSystemInformation}");
}
Tkael commented 2 years ago

Interesting observation, but perhaps it would be more beneficial to remove showInformation entirely. It is quite arguably redundant and inferred when we ask for stations or factions. 🤔 Thoughts?

boyestrous commented 2 years ago

Makes sense to me!

EDSM literally returns just the name of the system without showInformation, so it doesn't seem very realistic for someone to intentionally set showInformation to false.

I can pull it out and update this branch tomorrow morning, if that works for you?

Tkael commented 2 years ago

Yes please. And thank you. :-)

boyestrous commented 2 years ago

All tests passed except the OAuthClientID test, which doesn't appear to be related to this. It references the companion app, which I haven't played with yet.

The app builds and runs as expected!

Tkael commented 2 years ago

The OAuthClientID test is a preventative action to make sure that we don't release a build without a file we need for interfacing with the Frontier API (excluded from commits per request from FDev, though anyone that submits an application to FDev for Frontier API access would be able to use their own value in its place).