search

TotalWar-Modding / tw_pack_lib

1 stars 0 forks source link

Do PFHVersion's "Unsupported" variant and the pfh* preambles make sense? #7

Closed Trolldemorted closed 5 years ago

Trolldemorted commented 5 years ago

If you try to build a packfile with it it will fail, and parse_pack will return an error if the version is unsupported, so that variant is never actually used. The only usage of PFHN_PREAMble for N!=4 and N!=5 is in get_version, which is unreachable as well since you cannot open such a pack file.

Exhaustive enums are an awesome feature of rust, so we should use it and not clutter our enums with unused/default variants.

Frodo45127 commented 5 years ago

From inside the lib it doesn't. For someone using the lib it serves to report that X file is a PackFile but it's not yet supported, instead of saying just a generic invalid error that may make the user think "hey, this program corrupted my mod!" or something like that. Though yeah, it may be better to move the unsupported type to an error directly instead.

The PFH preambles are the ones used by shogun 2, napoleon and empire. Despite being valid PackFiles, they are PackFiles the lib cannot open, so an specific error for them is normal in that situation.

Trolldemorted commented 5 years ago

From inside the lib it doesn't. For someone using the lib it serves to report that X file is a PackFile but it's not yet supported, instead of saying just a generic invalid error that may make the user think "hey, this program corrupted my mod!" or something like that. Though yeah, it may be better to move the unsupported type to an error directly instead.

We already have an error for that - UnsupportedPackFile was meant to say exactly that (we might want to rename it to UnsupportedPackFileVersion or the like). We will never be able to return/build a PackFile if the version is unexpected/unsupported, so both enums' default variants are unused right now (?).

Default variants don't make much sense in general - if something can be not explicitly set, one can always use an Option<T>, and set it to None if the value can not yet be determined.

Frodo45127 commented 5 years ago

The enum was before the error, that's why there is an enum variant AND an error, but as I said, with the error the enum variant (PFHFileType::Unsupported) can be removed.

Frodo45127 commented 5 years ago

I just pushed a commit to remove the PFHVersion::Unsupported, so I guess this can be closed now.