-
Notifications
You must be signed in to change notification settings - Fork 245
fix(csharp): Fix examples fixture #9254
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
base: main
Are you sure you want to change the base?
Conversation
69da7c4
to
5255c85
Compare
generators/csharp/base/src/asIs/test/Json/AdditionalPropertiesTests.Template.cs
Show resolved
Hide resolved
|
||
// persist the state of the generator to allow subsequent tools to know what was generated | ||
await saveGeneratorState( | ||
join(this.absolutePathToOutputDirectory, RelativeFilePath.of(".csharp-generator-state.json.user")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does the .user
suffix mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D
I picked that because the .gitignore
has an exclusion for that so that if the generator state was still on disk, the file wouldn't end up in the SDK PR when pushed up to github.
@@ -0,0 +1,593 @@ | |||
/** | |||
* @fileoverview Type canonicalization utilities for C# code generation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like JSDoc mainly uses @file
and @fileoverview
is a synonym
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah! Cursor betrayed me! (fixing...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module looks fantastic. Can we encapsulate most of this in a class and include tests for its functionality?
For now, we can keep storing the data in the module scope, but wrapped in an instance of the class.
In the future we could use a DI container to register and retrieve this data.
// ensure that each response code is handled only once | ||
const handled = new Set<number>(); | ||
for (const error of endpoint.errors) { | ||
const errorDeclaration = this.context.ir.errors[error.error.errorId]; | ||
if (errorDeclaration == null || handled.has(errorDeclaration.statusCode)) { | ||
continue; | ||
} | ||
handled.add(errorDeclaration.statusCode); | ||
this.writeErrorCase(error, writer); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
// remove the generator state from the directory, since it is no longer needed | ||
unlink(generatorStatePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that necessary? I think it'd be easier to debug when not deleting this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that cleaning up the file was prudent - but maybe not a big deal. I can remove that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we could order the status code switch by the numerical value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Metadata = new SeedExamples.Metadata(new Metadata.Html("metadata")), | ||
CommonMetadata = new SeedExamples.Commons.Metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is FQN'ing necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two Metadata
types - but you're correct it does seem like it's a smidgen more cautious here than was absolutely necessary.
I was thinking of following up later with a PR after doing some comparisons with dotnet format
so I could find all the places that the generator is too cautious with using fully qualified names all over the place. There are a number of hardcoded assumptions sprinkled throughout the generator that it would be nice to nudge into using the canonicalization code and solve all at once.
Description
Linear ticket: https://linear.app/buildwithfern/issue/FER-6267/fix-c-seed-failures
Fixes the C# generator failures for the
examples
fixtureChanges Made
The PR is broken into three commits:
examples
are in theseed/csharp-sdk/examples/readme-config/src/SeedApi.DynamicSnippets
folderCreated disambiguation framework for generated C# types and namespaces:
The canonicalization/disambiguation framework tracks types and namespaces as they are
created and used, so that collisions can be detected early, and automatically adjust
the generated code to avoid conflicts. To this point, collsions in namespaces with names
of classes or conflicting namespaces will adjust the namespace by appending an underscore.
NOTE: this PR doesn't agressively add canonicalization to everywhere, it is being
incrementally added where changes are needed to support test cases. This can easily
be expanded in the future as needed.
It creates an in-memory registry of types, namespaces, typenames, and known identifiers
that can be checked against.
Notably:
ClassReference
instanceswith graceful fallback in browser environments.
To support typename/namespace disambiguation:
save the c# generator state at the end of creating the
CSharpProject
files so thatthe DSG can be aware of changed names to types and namespaces.
for each
Class
that gets created, theClassReference
for the type is trackedrewrote
ClassReference.writeInternal
to handle dismbiguation when writing out theclass reference into source code.
modified the
Writer.stringifyImports
be able to get the resolvedNamespace when writingout the
using
imports.created a
builtIn.ts
file that contains the namespaces and typenames for c# built-intypes so that the disambiguation can be aware of what names will be conflicting with
built-in types. (this data was initially generated in c# using reflection)
System
that has properties for creatingClassReference
instances for built-in types (will expand this later). This will help in the long term
to ensure that the disambiguation code can identify what has been used.
Modified the dynamic snippet generation to load the generator state if it exists
before generating any snippets.
Modified c# 'asis' templates:
Changed references to
Exception
to be globally explicit:global::System.Exception
This was required to support models that use the name Exception and the new namespace
disambiguation support in this PR isn't yet accessible to asis templates
modified the namespace declaration in the AsIs templates to get the test namespace
from the variable context (
testNamespace
) rather than letting it string-smash thetext together.
modified the
getTestNamespace
method to get the canonicalized namespace instead ofblindly string-smashing the text together.
cleaned up the variable passing to the templates to allow additional properties to be
passed in as template variables.
made the SdkGeneratorCLI build models before generating any other code to give types
that are exposed to the user the least likely chance to be in conflict.
the
SubPackageClientGenerator
canonicalizes theClassReference
for the client classbefore generating the code.
the
WrappedRequestGenerator
canonicalizes theClassReference
for the request classbefore generating the code.
Resolved bugs and build errors that are exposed by the
examples
generator:in the
EndpointSnippetGenerator
if a request body was marked optional and didn't havea value in the example, the generator would not generate a
null
(even though the parameteritself was not optional).
modified the
DynamicSnippetsGeneratorContext
to use the now-dynamically exapanding knownidentifier support.
fixed the
DynamicTypeLiteralMapper
to correctly generate the initializer for unions withdiscriminators. (it had been generating empty constructor calls)
the
DynamicTypeLiteralMapper
now knows when to fully qualify a type when it is usinga known identifier.
When
UnionGenerator
andRootClientGenerator
emits code to throw anException
itnow uses the fully qualified name of the
Exception
class when there is a name collisionwith a generated model.
fixed a bug in the HttpEndpointGenerator that would generate faulty code if the a response
status code was duplicated
the
XmlDocBlock
file was getting a circular dependency, changedClassReference
tobe
type
imported.Minor quality changes:
long
types by changing the suffix toL
instead ofl
Current known cosmetic issues:
it appears that the
JsonConverter
type is being fully expanded because it thinks thatthere is an ambiant name collsion. Will be follow up.
nested types for Enums are being slightly more verbose when generating
operator implicit
methods. (they are including the enclosing type name in the method signature). Will follow up.
Updated versions.yml
Testing
pnpm test:update
seed:local
andseed
with--local
and without) as during development I found a couple of cases where the behavior changed a bit between the two.