search

hohav / peppi

Rust parser for Slippi SSBM replay files
MIT License
37 stars 9 forks source link

Implement `Default` trait for `Version` #50

Closed NickCondron closed 2 months ago

NickCondron commented 1 year ago

Slippi replay base version is 0.1.0

A user might want to use Version to encode some sort of compatibility information in a type (eg. certain stats only work for certain replay versions). It's better to use Default::default than Version(0,1,0) because

  1. You don't have to remember which version is the earliest
  2. It more clearly communicates the intent to someone reading the code. For example, min_version: Default::default() more clearly communicates that all versions should work.
  3. Allows user types that include a Version to also derive default if it makes sense
hohav commented 1 year ago

Could you describe why we want this in the PR description?

hohav commented 8 months ago

You don't have to remember which version is the earliest

That assumes the default is the earliest version of Slippi, which doesn't make sense to me. If there even is a context where it makes sense to have a "default Slippi version", IMO the latest supported version of Slippi would be a more sensible and intuitive choice than the earliest version. Besides, I don't think remembering "0.1.0" is particularly hard, and we could even provide a const like MIN_VERSION if there's demand.

It more clearly communicates the intent to someone reading the code. For example, min_version: Default::default() more clearly communicates that all versions should work.

Very much disagree. In general, I would not assume a "default version" of something automatically means the earliest supported version. To me, min_version: Version(0, 1, 0) is clearer and less ambiguous.

Allows user types that include a Version to also derive default if it makes sense

When would that make sense, though? I can only think of cases where you'd want the latest version.

hohav commented 2 months ago

Closing for lack of activity. Feel free to reopen for further discussion.