NeVeSpl / NTypewriter

File/code generator using Scriban text templates populated with C# code metadata from Roslyn API.
https://nevespl.github.io/NTypewriter/
MIT License
126 stars 25 forks source link

Type.AllReferencedTypes includes types used by methods of the specified type. Is that what is desired? #21

Closed gregveres closed 3 years ago

gregveres commented 3 years ago

Following your suggestion in issue #8, I switched over to use Type.AllReferencedTypes.

Here is an example where too many types are returned. The import section is driven by the call to Types.AllRelatedTypes. As you can see, SessionBox and SeesionPlayer are not needed in this file. Ie, they are not directly related to BoxLeagueNextSession. They are related to BoxLeagueSession.

import { NextSessionBox } from './NextSessionBox';
import { MemberInfo } from './MemberInfo';
import { SessionBox } from './SessionBox';
import { SessionPlayer } from './SessionPlayer';
import { BoxLeagueSession } from './BoxLeagueSession';

export interface BoxLeagueNextSession extends BoxLeagueSession {
  nextSession: NextSessionBox[],
  nextSessionDateRangeDescription: string,
  nextSessionStartDate: string,
  nextSessionEndDate: string,
  targetBoxSize: number,
  playersLeaving: MemberInfo[]
}
NeVeSpl commented 3 years ago

Could you provide fragments of c# code of BoxLeagueSession type where SessionBox and SeesionPlayer are used?

gregveres commented 3 years ago

Sure. I will make them as small as possible but still capture what I have.

This is the class being exported

        [ExportToTypescript]
        public class BoxLeagueNextSession : BoxLeagueSession
        {
        }

BoxLeagueSession

        [ExportToTypescript]
        public class BoxLeagueSession
        {
            public int LeagueId { get; set; }
            public int SessionId { get; set; }
            public List<SessionBox> Boxes { get; set; }
            :
        }

all other members of BoxLeagueSession are primitive types.

SessionBox

       [ExportToTypescript]
        public class SessionBox
        {
            public List<SessionPlayer> Players { get; set; }
            public string Name { get; set; }
            public int boxId { get; set; }
            :
        }

Then interestingly, SessionPlayer includes another complex type, but that type isn't included in the above list of imports.

SessionPlayer

        [ExportToTypescript]
        public class SessionPlayer
        {
            public string Name { get; set; }
            public int Id { get; set; }
            public RoundRobinCompetitor.RoundRobinCompetitorStatus Status { get; set; }
            :
            public int StartingRank { get; set; }
            public List<SessionMatchResults> MatchHistory { get; set; }
        }

Notice that SessionPlayer includes SessionMatchResults but SessionMatchResults wasn't included in the list of imports. Not that I want SessionMatchResults to be included, I don't think SessionPlayer should have been included.

NeVeSpl commented 3 years ago

I am still not able to reproduce the bug, could you also show a fragment of the template that generates imports?

gregveres commented 3 years ago

Sure. And I just verified that I am still getting the output that I originally posted. (the template file has been through a lot of iterations in the last few days).

My output is exactly the same as what I originally posted.

Here is the template file:

{{- # Helper classes }}
{{- func ImportType(type)
    useType = Type.Unwrap(type)
    if (useType.ArrayType != null) 
      useType = useType.ArrayType
    end
    if (type.IsEnum) 
      path = "../Enums/"
    else
      path = "./"
    end
    if ((useType.Attributes | Array.Filter @AttrIsExportToTypescript).size > 0)
        ret "import { " | String.Append useType.Name | String.Append " } from '" | String.Append path | String.Append useType.Name | String.Append "';\r"
    end
    ret null
    end
}}
{{- func AttrIsExportToTypescript(attr)
    ret attr.Name | String.Contains "ExportToTypescript"
    end
}}

{{- # output classes }}
{{- for class in data.Classes | Symbols.ThatHaveAttribute "ExportToTypescript" | Array.Concat (data.Classes | Symbols.ThatHaveAttribute "ExportToTypescriptWithKnockout")
        capture output }}

{{- for type in (class | Type.AllReferencedTypes)}}
    {{- ImportType type}}
{{-end}}

export interface {{class.Name}}{{if class.HasBaseClass}} extends {{class.BaseClass.Name; end}} {
  {{- for prop in class.Properties | Symbols.ThatArePublic }}
  {{ prop.Name }}: {{prop.Type | Type.ToTypeScriptType}}{{if !for.last}},{{end}}
  {{-end}}
}
{{- end}}
{{- Save output ("Interfaces\\" | String.Append class.BareName | String.Append ".ts")}}
{{- end}}
gregveres commented 3 years ago

I don't know what timezone you are in, but if you give up for the night, I can take a crack at creating a unit test case that covers it and seeing if I can reproduce it in the NTypewriter project. Just let me know when you stop working on it for the day.

NeVeSpl commented 3 years ago

Yup I will need that unit test, or sample project uploaded on github. Some magic is happing here. Meantime I will upload version 0.0.8, maybe something wrong is with version 0.0.7

gregveres commented 3 years ago

I think I have figured out where the extra references are coming from.

When I pasted the classes, in the issue here, I did not include the methods. The two extra class references are used in return types of methods on BoxLeagueNextSession.

I went through all 403 generated interface files and only 2 of them had extra imports. Both of the classes being exported have functions on them that reference the extra class as a return type.

Now that I know where the issue is coming from, I am wondering if types referenced in methods should be included. I don't really know the answer. I don't use it that way but someone else might use it that way.

I then considered "should the function take an argument to specify the inclusion or exclusion of method types?". But since it was 2 files out of 403, I am not sure it matters to me. From a perfectionist point of view, I would like to not include them, but from a practical point of view, there are other things to get working first - like proper return types on controller actions.

I will leave it to you. Now you know why those extra types are there.

NeVeSpl commented 3 years ago

All referenced types should be returned, so it works as designed. But from the beginning, I was thinking about giving a possibility to opt-out from checking specific parts of a type.