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

Build options is an arbitrary json object rather than a string #183

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Oct 16, 2023

Fixes #180. I've verified that arbitrary JSON can be passed without error and parsed properly. It can also continue to be passed as a string.


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
src/Serval.Client/Client.g.cs 70.60% <100.00%> (ø)
...Translation/Contracts/TranslationBuildConfigDto.cs 100.00% <100.00%> (ø)
...erval.Translation/Contracts/TranslationBuildDto.cs 100.00% <100.00%> (ø)
...lation/Controllers/TranslationEnginesController.cs 91.43% <100.00%> (ø)

📢 Thoughts on this report? Let us know!.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to update the Options property on TranslationBuildDto.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)


src/Serval.Translation/Contracts/TranslationBuildConfigDto.cs line 8 at r1 (raw file):

    public IList<PretranslateCorpusConfigDto>? Pretranslate { get; set; }

    public object? Options { get; set; }

The Swagger docs show an invalid example for this property. Specifically, it shows a string value, which will result in a 400 status code. We should specify a valid example using code documentation. For example:

/// <example>
/// {
///   "property": "value"
/// }
/// </example>

@Enkidu93
Copy link
Collaborator Author

I had intentionally not done that, but I suppose there's no good reason not to. It would make the presentation cleaner and more consistent to the user when they request an existing builds information.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we want it to be consistent.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)

@Enkidu93
Copy link
Collaborator Author

src/Serval.Translation/Contracts/TranslationBuildConfigDto.cs line 8 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The Swagger docs show an invalid example for this property. Specifically, it shows a string value, which will result in a 400 status code. We should specify a valid example using code documentation. For example:

/// <example>
/// {
///   "property": "value"
/// }
/// </example>

OK, good call: I wasn't familiar with how to do that properly, but I wondered the same thing.

Copy link
Collaborator Author

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @ddaspit and @johnml1135)


src/Serval.Translation/Contracts/TranslationBuildConfigDto.cs line 8 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

OK, good call: I wasn't familiar with how to do that properly, but I wondered the same thing.

Done.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)


src/Serval.Translation/Contracts/TranslationBuildDto.cs line 22 at r2 (raw file):

    public JobState State { get; set; }
    public DateTime? DateFinished { get; set; }
    public object? Options { get; set; }

You should add the example here as well.

@Enkidu93
Copy link
Collaborator Author

src/Serval.Translation/Contracts/TranslationBuildDto.cs line 22 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You should add the example here as well.

Done.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

@Enkidu93 Enkidu93 force-pushed the build_options_is_object branch from 0fffaf6 to 8e082a6 Compare October 16, 2023 18:51
@Enkidu93 Enkidu93 merged commit b0643ec into main Oct 16, 2023
1 check passed
@Enkidu93 Enkidu93 deleted the build_options_is_object branch October 16, 2023 19:34
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

Successfully merging this pull request may close these issues.

The build options passed to the start build endpoint should be arbitrary JSON
3 participants