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

Strict parsing #142

Closed
wants to merge 3 commits into from
Closed

Strict parsing #142

wants to merge 3 commits into from

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Sep 25, 2023

This change is Reviewable

@johnml1135
Copy link
Collaborator

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 792 at r1 (raw file):

        {
            return BadRequest(ae.Message);
        }

There should really be an explanation for options in the swagger docs. Possibly something like this:
general:

  • options are a json file that changes how the engine is built.
    • General options:
      • strictPasring: throw an error if the corpora are not properly formatted
    • There are no SMT specific options at this time
    • There are no NMT specific options at this time

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

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.

I think there was a misunderstanding about the strict parsing flag. I did not intend for the flag to be configured by the client. What I meant is that it would always be set to true for Serval but would be set to false by default, so that applications using Machine as a library would continue to get the same behavior.

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


src/Serval.Translation/Services/EngineService.cs line 200 at r1 (raw file):

                EngineId = engine.Id,
                BuildId = build.Id,
                Options = build.Options ?? "",

Options is optional, so you shouldn't set it if it is null.

@Enkidu93 Enkidu93 closed this Sep 26, 2023
@Enkidu93 Enkidu93 deleted the strict_parsing branch October 10, 2023 17:47
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