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

Generics not working as expected when decorator uses ResolveLocalPath #74

Open
myshkin5 opened this issue Jun 18, 2023 · 5 comments
Open

Comments

@myshkin5
Copy link
Contributor

When using a decorator with ResolveLocalPath set to true with generics, the path of the generic type is returned as the package where the type is located. I think it should always be empty string.

@myshkin5
Copy link
Contributor Author

I have a failing test in #75 and I'm looking for pointers on how to fix it. I'm thinking that I need to inspect parent in fileDecorator.resolvePath. The problem I'm seeing is that I'm at least four levels down in decorateNode at that point (function type, params field list, field, then id).

@dave
Copy link
Owner

dave commented Jun 18, 2023

I think it should always be empty string.

I haven't worked on this codebase for years so it's definitely possible I'm misunderstanding, but why? What's wrong with treating it as a local type?

If you're just trying to identify it as a generic type, then indicating this by having an empty string in the package path seems like a bad idea. e.g. when ResolveLocalPath==false, then you wouldn't be able to tell a generic type from a local type...

Or maybe I'm missing something?

@dave
Copy link
Owner

dave commented Jun 18, 2023

The problem I'm seeing is that I'm at least four levels down

Exactly... dst is concerned with the syntax, and is by design relatively dumb... it feels to me like determining whether a type is local or a generic type is perhaps something that requires a higher abstraction that dst is designed to do.

Remember, the ast package definitely can't tell you anything about types, and dst is supposed to replicate this as closely as possible.

The path resolving features are only supposed to make it possible to automatically manage the imports block. I feel that having generic types look just like local types will be ok...

Let me know what you think!

@myshkin5
Copy link
Contributor Author

I get it. I've implemented a type cache and I can repeatedly go back to the cache to see if a type lives in a context. I just have to keep track of what the context is.

I think this issue is still a concern -- using ResolveLocalPath causes type params to be reported incorrectly. Not sure anyone else is using ResolveLocalPath besides myself. I can update my PR to rip out the option if you would like. Or I can just leave it as is and get on with working around my issue.

Let me know how you would like to proceed. Thanks!

@dave
Copy link
Owner

dave commented Jun 19, 2023

Use your work-around for now, but we'll keep the ticket open and if lots of people have the same problem, we'll look at fixing it... I would need to spend a while re-acclimatising myself with the codebase because I haven't worked on this project for years.

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

No branches or pull requests

2 participants