Skip to content

Conversation

@SnipUndercover
Copy link
Member

ModAsset.Deserialize<T>() uses ModAsset.TryDeserialize<T>(out T), which suppresses all exceptions when deserializing YAML.
This can hinder debugging if deserialization is failing for whatever reason, as the exception info is now lost.

This PR causes ModAsset.Deserialize<T>() to now throw exceptions, if deserialization fails.

(Mods depending on ModAsset.Deserialize<T>() could break because of that, which is why this is a draft.)

@maddie480
Copy link
Member

I guess we need to search for mods using ModAsset.Deserialize to pull this PR out of review hell?

@maddie480 maddie480 added discussion Needs input from others and removed ready for review labels Feb 14, 2025
@maddie480
Copy link
Member

I decompiled all mods and none of them seem to use ModAsset.Deserialize, so uh sounds good to me

@maddie480 maddie480 marked this pull request as ready for review April 19, 2025 19:25
@maddie480
Copy link
Member

nevermind, I grepped .Deserialize instead and found quite a few ones

@maddie480 maddie480 marked this pull request as draft April 19, 2025 19:50
@maddie480
Copy link
Member

  • SkinModHelper return skinConfigYaml.Deserialize<SkinModHelperConfig>();
  • Styline return Everest.Content.Get("Content/" + data.Attr(attrName)).Deserialize<T>();
  • Eevee_Skinmod paletteList.Add(child.PathVirtual.Substring(child.PathVirtual.LastIndexOf('/') + 1), child.Deserialize<Palette>());
  • AchievementHelper List<Achievement> list = asset.Deserialize<List<Achievement>>();
  • SkinModHelperPlus return asset.Deserialize<T>();

@SnipUndercover
Copy link
Member Author

  • SkinModHelper: The helper does not check for nulls and will NRE if deserialization fails.
  • Styline: Here it uses Deserialize twice, but I'm not sure whether it will NRE, probably best to poke @Popax21 about it?
  • Eevee: The skinmod does not check for nulls and will NRE if deserialization fails.
  • AchievementHelper: The helper does not check for nulls and will NRE if deserialization fails.
  • SkinModHelperPlus: A move to TryDeserialize would help here.

An explicit exception seems like an overall benefit for SMH, the Eevee skinmod and AchievementHelper, so I'd leave them be.

@maddie480 maddie480 added review needed This PR needs 2 approvals to be merged (bot-managed) and removed discussion Needs input from others labels May 17, 2025
@maddie480-bot maddie480-bot added draft This PR is not ready for review yet (bot-managed) and removed review needed This PR needs 2 approvals to be merged (bot-managed) labels Jun 7, 2025
@SnipUndercover
Copy link
Member Author

Should this PR be revived? Do we have any other cases of ModAsset.Deserialize? @maddie480

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

draft This PR is not ready for review yet (bot-managed)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants