-
Notifications
You must be signed in to change notification settings - Fork 2
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
🌿 Fern Regeneration -- August 1, 2024 #37
Conversation
/// </summary> | ||
[JsonPropertyName("context")] | ||
[JsonConverter(typeof(OneOfSerializer<OneOf<string, Dictionary<string, object?>>>))] | ||
public OneOf<string, Dictionary<string, object?>>? Context { get; set; } |
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 should be a string
or an object
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.
We do the same thing in java for you
Happy to talk through possible workarounds, but we consider map<string, object>
to be a helpful type for object
, since a json object is of course a map of strings to values
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'm not sure what the most natural way is in Java, but in C# the most natural way would be to create an anonymous object:
new {
foo = "bar",
outer = new {
inner = "value"
}
}
A dictionary is helpful, but the DX is better for C# devs to use an object 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.
I see, that does seems ideal for object creation
Is this Context
type used as a request type? What do you think of this type as a response type, as it's shown 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.
This is only used as a parameter, it isn't returned anywhere.
Hypothetically, if it were returned, it'd be best returned as a string, or have a method overload to specify a type to convert it to using our JSON serializer.
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.
Oh my mistake 😅 going to look into making these changes
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.
Right now, since we don't differentiate between requests or responses when generating types, so they will all be given the same typings (given the same type definition).
So, if we made this change, how would you feel if OpenAPI object
type responses came through as object
rather than Dictionary<string, object?>
? That seems possibly harder to work with?
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.
For the request, we can do object
which you can assign a string to or anything else.
OneOf<string, object>
is fine too.
namespace AssemblyAI.Realtime; | ||
|
||
[JsonConverter(typeof(StringEnumSerializer<Realtime>))] | ||
public enum Realtime |
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.
Not sure why this is still being generated across the SDKs
|
||
public record ListTranscriptParams | ||
{ | ||
/// <summary> | ||
/// Maximum amount of transcripts to retrieve | ||
/// </summary> | ||
public int? Limit { get; set; } | ||
public long? Limit { get; set; } |
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.
Was this change intentional? I expected it to default toint
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.
we did make a change to represent int64
format OpenAPI integer
s as long
, as this is actually how long
s are represented in OpenAPI:
from here
here's your type in openapi for reference:
ListTranscriptParams:
x-label: List transcript parameters
type: object
x-fern-sdk-group-name: transcripts
# Don't use this type in Fern SDKs as it is already generated from the endpoint parameters
x-fern-ignore: true
additionalProperties: false
properties:
limit:
x-label: Limit
description: Maximum amount of transcripts to retrieve
type: integer
format: int64
minimum: 1
maximum: 200
default: 10
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.
Strange, I didn't think we specified that for this param. The maximum
is 200 so it doesn't make sense to say its int64
.
I'll update this in the spec. Thanks!
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.
Sounds good!
|
||
public record WordSearchParams | ||
{ | ||
/// <summary> | ||
/// Keywords to search for | ||
/// </summary> | ||
public string? Words { get; init; } | ||
public IEnumerable<string> Words { get; set; } = new List<string>(); |
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.
We should use Enumerable.Empty<string>
as default values
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 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.
The reason I suggest using Enumerable.Empty
is that it'll cache and reuse the object, reducing memory usage.
I didn't realize this, but .NET will automatic translate the collection expression to Enumerable.Empty
, so yes, let's do 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.
Awesome, thanks for this suggestion! I'll make the change 👍
|
||
#nullable enable | ||
|
||
namespace AssemblyAI.Core; |
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.
We don't want to expose a Core
namespace, so let's put this at the root namespace.
src/AssemblyAI/Core/Constants.cs
Outdated
@@ -1,6 +1,6 @@ | |||
namespace AssemblyAI.Core; | |||
|
|||
internal static class Constants | |||
public static class Constants |
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.
Let's make anything that the user doesn't need internal, or we'll need to make a lot of breaking changes version changes.
src/AssemblyAI/ClientOptions.cs
Outdated
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 need to keep using this custom client options which has set
instead of init
, and includes the API key.
These custom client options are used for the DI extensions.
|
||
#nullable enable | ||
|
||
namespace AssemblyAI; | ||
namespace AssemblyAI.Files; | ||
|
||
public partial class FilesClient |
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.
All clients should be partial
so we can extend them. I manually changed it to partial here.
switch (response.StatusCode) | ||
{ | ||
case 400: | ||
throw new BadRequestError(JsonUtils.Deserialize<Error>(responseBody)); | ||
case 401: | ||
throw new UnauthorizedError(JsonUtils.Deserialize<Error>(responseBody)); | ||
case 404: | ||
throw new NotFoundError(JsonUtils.Deserialize<Error>(responseBody)); | ||
case 429: | ||
throw new TooManyRequestsError(JsonUtils.Deserialize<Error>(responseBody)); | ||
case 500: | ||
throw new InternalServerError(JsonUtils.Deserialize<Error>(responseBody)); | ||
case 503: | ||
throw new ServiceUnavailableError(JsonUtils.Deserialize<object>(responseBody)); | ||
case 504: | ||
throw new GatewayTimeoutError(JsonUtils.Deserialize<object>(responseBody)); | ||
} |
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.
For exceptions that have a JSON error response, we want the exception message to be set to the .error
message.
Having the error code is helpful too (as a property) but shouldn't be included in the message.
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.
we want the exception message to be set to the .error message.
We are doing this today, no? I may be misunderstanding 😄
Do you mean that you want the .error
message to be the Exception
message?
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.
If I catch the exception, this should be true:
Assert.That(ex.Message, Is.EqualTo("each transcript source id must be valid"));
Currently, the ex.Message
is "BadRequestError".
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.
Hey @Swimburger , this ask makes a lot of sense, but it's a heavy lift on our end, as it would be a new end to end feature
Would it be possible to remove this from the GA blockers, and we'll follow up with it after GA?
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.
As I suggested in a past conversation, we don't need this to be something that Fern generates, but we do need an extension point where we can add our own code without disabling too much 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.
You can provide a partial method for us to implement like I show here:
https://github.com/AssemblyAI/assemblyai-csharp-sdk/pull/39/files#diff-cadbe42714ba9be588f87ec2595828c853eed16be025736993d2c370bb9e6e1aR10
switch (response.StatusCode) | ||
{ | ||
case 400: | ||
throw new BadRequestError(JsonUtils.Deserialize<Error>(responseBody)); |
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.
These exception classes should be suffixed by Exception
.
However, we could just have a single exception for all of these.
We don't need a separate exception class for each error.
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 Fern generated dedicated classes for our unions like this.
new OneOfSerializer<OneOf<LemurStringResponse, LemurQuestionAnswerResponse>>() | ||
}, | ||
WriteIndented = true, | ||
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, |
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 had to configure this, so that null
properties are omitted from the JSON.
Otherwise there's an error in the API.
The API should be able to handle null values, but it doesn't properly for some properties, which has been reported.
/// </summary> | ||
public object Body { get; } = body; | ||
|
||
public override string ToString() |
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.
Exceptions shouldn't override the ToString
method.
To communicate the exception details, use Message
and additional properties only.
@@ -1,6 +1,6 @@ | |||
namespace AssemblyAI.Core; | |||
|
|||
public class Environments | |||
public class AssemblyAIClientEnvironment | |||
{ | |||
public static string DEFAULT = "https://api.assemblyai.com"; |
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.
DEFAULT
should be Default
according to .NET naming conventions
$"Error with status code {response.StatusCode}", | ||
response.StatusCode, | ||
JsonUtils.Deserialize<object>(responseBody) | ||
); | ||
} | ||
|
||
/// <summary> | ||
/// Question & Answer allows you to ask free-form questions about a single transcript or a group of transcripts. |
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 not legal in XML docs. The contents of the XML docs should be XML escaped.
This PR regenerates code to match the latest API Definition.