Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove TS::trasparent #243

Merged
merged 4 commits into from
Feb 29, 2024
Merged

Remove TS::trasparent #243

merged 4 commits into from
Feb 29, 2024

Conversation

escritorio-gustavo
Copy link
Contributor

This PR is meant to remove the TS::transparent method, as it was rendered useless by #241, always return false with the exception of Range and RangeInclusive

Copy link
Collaborator

@NyxCode NyxCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this was much much more straight-forward than I thought. Awesome!

ts-rs/src/lib.rs Outdated
Comment on lines 682 to 689
fn generics() -> impl TypeList
where
Self: 'static,
{
use std::marker::PhantomData;
((PhantomData::<I>,), I::generics())
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is equivalent to just returning I::generics().
I'm not sure that's correct, even though the tests pass.
Shouldn't this return ().push::<I>().extend(I::generics())? Right now, I itself would be missing from the TypeList, no?

Ahhh, I got confused by you constructing the TypeList manually here!
Is there a reason you're not doing ().push::<I>().extend(I::generics()) or, even better, I::generics().push::<I>, instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, I just kinda got confused calling methods on () so I did it this way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just kinda got confused calling methods on () so I did it this way

That tripped me up quite a bit in the macros, because the repetition syntax uses parentheses plus the unit type plus the calling of methods

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah, I understand.
I do think it'd be a good idea to keep the implementation of TypeList private, and use the methods on it. That gives us the option to change TypeList is implemented however we like in the future.

In this particular case, I::generics().push::<I>() would probably be the nicest.
To make the code more readable (and keep the implementation detail that the empty TypeList is () hidden), we could add a const EMPTY_TYPELIST: () = () or pub fn new() -> impl TypeList {} to typelist.rs

For the macros, I try to use quote!{} or quote![] to disambiguate them, but yeah, there's still the #()* syntax.

In the end, this is not a big deal, if you've got strong opinions here. Personally, I prefer using the methods on the type.

Edit: I see that you already changed it, thanks! 😆

Being able to call methods on TypeList was the motivation to make dependency_types a function in the first place. It could have been an associated type (e.g type Dependencies = ((A), B), C::Dependencies)), but I liked the idea of being able to pretend that TypeList is just a Vec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of that makes a lot of sense! I'll open another PR to apply this change to the impls I changed in #241, all of which manually build a TypeList

Comment on lines -370 to 372
if T::transparent() {
// the dependency `T` is "transparent", meaning that our original type depends
// on the dependencies of `T` as well.
T::dependency_types().for_each(self);
} else if let Some(dep) = Dependency::from_ty::<T>() {
// the dependency `T` is not transparent, so we just add it to the output
if let Some(dep) = Dependency::from_ty::<T>() {
self.0.push(dep);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that this just works. Awesome!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, since T::transparent is always false we just get rid of that check

@escritorio-gustavo escritorio-gustavo merged commit a05169e into main Feb 29, 2024
10 checks passed
@escritorio-gustavo escritorio-gustavo deleted the remove-transparent branch February 29, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants