Implement Space Age

[IanWhitney]

First version that passes the tests.

This first commit is intended so we can discuss the crate's API and the tests. There are two decisions I made here that I can see people finding odd.

1. Creating each class as a separate struct.
   • I did this because I want students to learn about traits. Obviously they'll have do deal with the From trait, but I can also see implementations grappling with shared behavior between the various planets that can be handled via traits.
2. Testing that a struct is equal to a float This was dumb

I'm not bound to either of these decisions, but they were intentional.

To Do Before Merge

• Confirm API makes sense
• Add test helper that can compare floats
• Update example code to implement From<u64> and From<f64> generically Skipping, not necessary and probably requires the num crate anyway.
• Can we add to Readme with links to Traits & From?
• Placing it in the problem order

[IanWhitney]

I wouldn't spend much time looking at the example code yet. It's not meant to be good. Just there to get the tests passing.

[petertseng]

I have observations, but not necessarily positions on how things should be done. I will give the obesrvations, and maybe come back in a day with positions when I have had time to think about them.

Testing that a struct is equal to a float

Currently, we would have to accept that the example's partial equivalence is not symmetric, which is contrary to https://doc.rust-lang.org/core/cmp/trait.PartialEq.html .

We can see that if we changed one of the assertions from assert_eq!(age, 31.69); assert_eq!(31.69, age); it would be a compilation error! Well, we would fix the error if the example would also define impl PartialEq<EarthAge> for f64, etc.

Although the symmetricity would not get tested unless we explicitly have assertions both ways in our tests. So I guess one question is "the tests don't require that the partial equivalence is symmetric; should they?"

The same goes for transitivity if f64 defines its equivalence to other types - then for the partial equivalence to be transitive *Age would have to define its equivalence to those other types as well. But would we even want to test that?

Huh. This gets quite verbose quite the more types we wish to define as equal to one another, since O(n^2) pairs of eq functions need to be defined, it seems...! I wonder how others have dealt with this problem?

I wonder what it would be if the various *Age were equatable to just each other, though maybe that would make it deriveable, contrary to one of the stated goals.

[IanWhitney]

Yeah, I had already decided that my Value Object idea was terrible. This is just more evidence to support that. I'll switch that part of the implementation.

[petertseng]

BUT YOU FORGOT PLUTO

I am obviously joking, but I am (and I guess that you are also) old enough to have grown up with Pluto being a planet.

I will attempt the exercise myself and see what I think of the idea of using this exercise for traits. The stub file is very verbose. I will attempt to curb the verbosity when I implement, but of course we can't guarantee that all students will think this way. Perhaps that has to be left to discussion on the site.

If I can't find a way to curb the verbosity, then I would question the choice to make each *Age a different type.

[petertseng]

Life would probably be better if there was a tolerance on the expected values

thread 'earth_age' panicked at 'assertion failed: (left == right)(left:31.688087, right: 31.69)', tests/space-age.rs:8

I don't think I should be required to round that

[petertseng]

the #[derive(Debug)] can probably be removed?

[petertseng]

The least-repetitive I came up with: https://gist.github.com/petertseng/0b3928f0ee8881e94bf654d3f2153df9

Repeating X and XAge on the same line is unfortunate, but I didn't find a way around it.

It obviously fails the tests because it doesn't round, but it would ucceed if it did (or if the tests didn't care)

Not necessarily an advocation for putting this in the example solution. Wonder what others can come up with to reduce duplication first.

[IanWhitney]

Macro rules. Interesting! Never occurred to me.

I think my approach looks something like this:

https://gist.github.com/9a9ddb50afd08f4ae5a20258c056916b

My least favorite part of it is that the Planet trait has to define a seconds function that ends up being an alias for the seconds value. Maybe there's a way around that, but I don't know what it is.

I will also have to implement From<u64> for each individual planet, right? Though maybe there's a way to make that generic over each T: Planet?

Questions like that are why I structured the tests the way I did, honestly. I want to push students towards thinking about generics, traits and code reuse. To me that's how we make this problem interesting and valuable to people learning Rust.

[petertseng]

Macro rules. Interesting! Never occurred to me.

I only use them to take care of the duplication that I could not avoid.
All other duplication was avoided with the Planet trait.
Someone could ostensibly write the entire thing in a macro (From<u64> and years for each individual planet).
I would consider that bad style and too much to put in a macro: If it can be done outside of a macro, do so.
(But enough soap-boxing from me)

I will also have to implement From<u64> for each individual planet, right? Though maybe there's a way to make that generic over each T: Planet?

There absolutely is. See the impl <T: Planet> From<u64> for Age<T> if you need, though I would encourage a search for other solutions in addition - perhaps there are other ways! The desire to avoid that duplication was what drove me to start my solution.

Questions like that are why I structured the tests the way I did, honestly. I want to push students towards thinking about generics, traits and code reuse. To me that's how we make this problem interesting and valuable to people learning Rust.

You certainly got me interested and made me learn a few things, so you have me convinced.

[IanWhitney]

Over the course of several hours I worked on a long comment explaining why this approach wouldn't work. Turns out I rubber ducked myself

I think this does what I want

https://play.rust-lang.org/?gist=9e1acfeae7ebfdc7dd8176c1c015adb9&version=stable&backtrace=0

It's pretty different from the first approach. After working on this a few ways, I realized that at its core, the problem has a couple of concepts

• Durations (seconds, orbital lengths [i.e., years])
• Planets

The language of the exercise makes you think about "Years" as a property of a person, or some other object separate from a Planet. So I kept trying to put that logic somewhere outside of Planet.

But a "year" is the same as a planetary orbit, and it makes perfect sense to ask a planet how many orbits it's made in a duration. Hence the orbits_during function. And the trait can easily provide a default implementation for that function that works for all <T: Planet>

I would like to combine the 2 implementations of From for Duration , but found differing advice on how to declare a function generic over numeric types.

[IanWhitney]

Life would probably be better if there was a tolerance on the expected values

Looking at the Ruby implementation they can use the assert_in_delta method. Rust doesn't have that, but it's easy enough to add.

[IanWhitney]

I've updated the implementation. And I've put a To Do list at the top of the PR so that I can track progress.

[IanWhitney]

Problem order specified. I think its current position is OK.

[petertseng]

I suppose I'm not afraid to add a dependency if it's necesary (that's what cargo is for) but I want to hear the explanation for why it is necesary, rather than have fn assert_in_delta(expected: f64, actual: f64). What does the bound of Float get us that we will not otherwise have? Does it allow students more flexibility in the return type of their years_during function?

[IanWhitney]

Maybe there's an aspect of Rust I'm missing here. I want this function to accept both f32 and f64 and the only way I could see to do that is by using the Float trait in num

Also, the NumCast part of num comes in pretty handy.

[petertseng]

I want this function to accept both f32 and f64

Is that because you think students may change years_during to return an f32 instead of an f64 (as it is so defined in the stub file) and you want to be prepared for this situation?

[IanWhitney]

Hmm. I swear I was having problems with comparisons in some cases, but now I can't re-create them. I'll drop the crate and see if I run into any problems.

[IanWhitney]

One of the to-do items has baffled me

Update example code to implement From and From generically

I've tried tackling this a variety of ways, all with no luck. Anyone know how to implement From for all numeric types? I would thinks something like:

impl<T: Num> From<T> for Duration {
  fn from(s: T) -> Self {
    Duration { seconds: s }
  }
}

Would work. But no. I get "non-scalar cast: T as f64". I think I'm getting the syntax wrong, but I'm not sure how.

[IanWhitney]

I'm on the fence about 81fa6ee. Interested in what others think of the idea.

If we're OK with the API then this should be ready to go.

[petertseng]

I know you aren't thinking about the genericity over u64 and f64 anymore, but you weren't alone on that "non-scalar cast": rust-lang/rust#31174 (comment)

I do suppose the num crate would have done it, but then we'd be including the num crate for the example only (it's not used by the test), and I wonder whether it would confuse students to see the crate and wonder whether they have to use it. We're not the only track with this problem either.

I have no complaints about the problem placement or topics.md

I imagine there may be questions like "why isn't Planet just an enum and the interface years_on(p: Planet, seconds: u64)?!" or "does it make sense that they're separate types if there's only one of each panet", but that will probably have to be the way things be if taking this approach. I'm not opposed.

[petertseng]

interesting point that Earth.years_during is used rather than Earth::years_during. This is possible because Earth is a struct with no members (a unit-like struct in the nomenclature). Doesn't seem to me like these functions need to take a self though...

[petertseng]

This allows for the "unusual" solution of https://gist.github.com/petertseng/ec3887c899f0259661847e8fbfba820d

[IanWhitney]

I imagine there may be questions like "why isn't Planet just an enum and the interface years_on(p: Planet, seconds: u64)?!" or "does it make sense that they're separate types if there's only one of each panet", but that will probably have to be the way things be if taking this approach. I'm not opposed.

I think we're always welcoming of suggested changes like this. We can always point to PRs like this one (or the commit history) to say, "Here was the goal of this API". And if people can provide a good alternative that satisfies the same goals, then all the better.

interesting point that Earth.years_during is used rather than Earth::years_during. This is possible because Earth is a struct with no members (a unit-like struct in the nomenclature). Doesn't seem to me like these functions need to take a self though...

Agreed that self is unnecessary here. Commit incoming.

[IanWhitney]

Ok. I think this one is winding down so I've rebased down to a single commit. I'll let this PR sit until Friday, August 19th. If nothing comes up, I'll merge it.