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

Fixes Throw error for unbuilt engine #81 #82

Merged
merged 6 commits into from
Sep 12, 2023

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Sep 11, 2023

This change is Reviewable

@Enkidu93
Copy link
Collaborator Author

I've switched to Aborted so no changes are needed on the Serval side.

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 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)


src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs line 225 at r2 (raw file):

        TranslationEngine engine = await GetEngineAsync(engineId, cancellationToken);
        if (engine.BuildState != BuildState.None || engine.BuildRevision == 0) //TranslationEngine.Revision - I don't see it used anywhere
            throw new RpcException(new Status(StatusCode.Aborted, "The engine must be built first"));

You should not throw an RpcException from here. RpcException is a gRPC-specific exception. This layer of the system should not be coupled to gRPC. You should probably create a new EngineNotBuildException or something like that.

@johnml1135
Copy link
Collaborator

src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs line 217 at r2 (raw file):

        TranslationEngine? engine = await Engines.GetAsync(e => e.EngineId == engineId, cancellationToken);
        if (engine is null)
            throw new InvalidOperationException("");

Shouldn't this throw a better error message? Like "Couldn't find engine 12345"?

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93)

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.

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


src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs line 217 at r2 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Shouldn't this throw a better error message? Like "Couldn't find engine 12345"?

I added one. It is caught almost immediately, but even so, it seems like it couldn't hurt to include a message.


src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs line 225 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You should not throw an RpcException from here. RpcException is a gRPC-specific exception. This layer of the system should not be coupled to gRPC. You should probably create a new EngineNotBuildException or something like that.

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 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

@Enkidu93 Enkidu93 merged commit 6716915 into master Sep 12, 2023
3 checks passed
@Enkidu93 Enkidu93 deleted the throw_error_on_unbuilt_engine_#81 branch September 12, 2023 20:20
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.

3 participants