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

Generic type parameter being removed while using ConvertToCSharpSource #92

Closed
eestein opened this issue Oct 29, 2021 · 7 comments
Closed
Assignees
Labels
Bug issues that suggest a flaw with existing code

Comments

@eestein
Copy link

eestein commented Oct 29, 2021

While using ConvertToCSharpSource on a parameter's type:

parameterInfo.ParameterType.ConvertToCSharpSource(true)

The type parameter is being excluded:

image

Here's the original code:

Expression<Func<TEntity, bool>> predicate = null

As you can see, TEntity has been removed.

What seems weird is that the Type contains the TEntity reference:

image

And that the same call to a method's return type includes the type parameter:

method.ReturnType.ConvertToCSharpSource(true)

image

@ZacharyPatten I'm filing this as a bug, because it seems like so, but please let me know if there's something wrong I'm doing or expecting...

Thanks.

EDIT: just as an extra info (I'm debugging, trying to find out where the problem is), the generic arguments seem to be all there:

image

@eestein eestein added the Bug issues that suggest a flaw with existing code label Oct 29, 2021
@ZacharyPatten ZacharyPatten self-assigned this Oct 29, 2021
@eestein
Copy link
Author

eestein commented Oct 29, 2021

@ZacharyPatten Ok, I think I found the problem:

image

For the first iteration showGenericParameters is true, but for the second one it's false:

image

ConvertToCSharpSource on line 557 is calling public static string ConvertToCSharpSource(this Type type, bool showGenericParameters = false) not the inner function, you should probably add the showGenericParameters down the chain if that was intentional.

@ZacharyPatten
Copy link
Owner

ZacharyPatten commented Oct 29, 2021

Thanks for the info, I was able to replicate the issue. This should be an easy fix.

It may be as simple as changing ConvertToCSharpSource(correctGeneric) to ConvertToCSharpSource(correctGeneric, showGenericParameters) on this line:

: (firstIteration ? string.Empty : " ") + ConvertToCSharpSource(correctGeneric));

But I need to do a bit more testing to validate.

The unit tests are here:

public void Type_ConvertToCsharpSource()

So I'll be creating a new branch and adding a/some unit tests to cover this topic.

@eestein
Copy link
Author

eestein commented Oct 29, 2021

Thanks!
Once you push the fix, is a new version automatically released? Just so I know when to update.

@ZacharyPatten
Copy link
Owner

ZacharyPatten commented Oct 29, 2021

Once the fix is ready I will create a release in the GitHub repo. There is a GitHub action that when a release is created on the repo, it will deploy a nuget package. It takes ~5-10 minutes for the nuget package to show up, so give it a few minutes after the release in the repo is created and you should see it in nuget.org and package explorer.

GitHub Action for nuget package deployments: https://github.com/ZacharyPatten/Towel/blob/main/.github/workflows/Towel%20Deployment.yml
Towel Releases: https://github.com/ZacharyPatten/Towel/releases

@eestein
Copy link
Author

eestein commented Oct 29, 2021

Thank you! :)

@ZacharyPatten
Copy link
Owner

ZacharyPatten commented Oct 29, 2021

@eestein I pushed an update. If the update did not fix your issue we can re-open this issue.

To be honest I can't remember why I decided to make the showGenericParameters parameter false by default. As I'm thinking about it now... most people probably want it true by default. When it is false there can be some odd behaviors like:

#warning TODO: review this test case

So there is probably additional improvement that can be done to to code.

Regardless, thank you for addressing this issue. :)

@eestein
Copy link
Author

eestein commented Oct 29, 2021

I agree it makes sense to keep it true by default, that's how I'm using it anyway :D

Thank you for the quick fix, I've already tested it and it's working!

I just ran into another issue now, but I'm debugging to see if the problem is on my end. I'll open another bug, if necessary.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug issues that suggest a flaw with existing code
Projects
None yet
Development

No branches or pull requests

2 participants