rbx_reflection: Superclass Iterator

[krakow10]

I thought it was weird that database.superclasses() returns a Vec rather than an iterator, perhaps it should be replaced or supplemented by something like this?

[Dekkonot]

I'm open to this conceptually, but not with the provided implementation; I've left a review of it.

In the future, try to open issues before making a pull request so we can determine if something is even something we want to do.

[Dekkonot]

Would love a test (nothing too complicated, just enough to verify it works and includes the starting value and the ending values), and this needs a changelog entry.

Otherwise, it looks good now.

[krakow10]

I'm open to this conceptually, but not with the provided implementation; I've left a review of it.

In the future, try to open issues before making a pull request so we can determine if something is even something we want to do.

I figure suggesting an idea with code is better since it can be immediately iterated upon. You're saying I should post a patch as a comment? Or push to my fork and link the branch maybe?

Thanks for the review, the code I had was rather tragic.

[Dekkonot]

What I mean is that you got lucky with this PR! There's a chance that I would have just said "no" and closed it, and your effort would've been wasted.

In the interest of everyone's time, I'd prefer we have issues where we at least go "yeah ok" before implementing things so that nobody's time is wasted.

[krakow10]

There's a chance that I would have just said "no" and closed it, and your effort would've been wasted.

An acceptable loss to me 😃

My mind's compiler says this implementation would miss the final ClassDescriptor ("Instance"). My first implementation before the PR would have missed the first one (the descriptor passed to the function), which I resolved by making the descriptor optional, hence the compilation mistake. I will look around the code and see what I can do about writing a test.

[krakow10]

My mind's compiler says this implementation would miss the final ClassDescriptor ("Instance")

I was correct. Here is a version that works, doesn't look horrible, and has a nice test.

[krakow10]

Side question... why does the superclasses function return an Option<Vec<_>> instead of just Vec<_>?

[Dekkonot]

Side question... why does the superclasses function return an Option<Vec<>> instead of just Vec<>?

Honest answer: It used to accept a &str as an argument and then I forgot to change it from a Option<Vec<_>> to a Vec<_>. I'm going to pretend that it's because you could construct a ClassDescriptor that doesn't exist in the database though, since that feels like it's more deliberate and makes sense. ;)

[Dekkonot]

Good enough. Thanks for the contribution!

[krakow10]

Is it also a good enough time for a release? I'm hoping to drop the patched packages I've been maintaining now that UTF8 conversion is merged.

[Dekkonot]

I can make a new release in a few days if it's necessary; I generally don't cut them until we need them, but if it's a blocker for you I can make it a priority.

[krakow10]

All my packages are compiling fine, so it'd just be a convenience to me. You're welcome to release whenever you feel is best.