Skip to content

Strict parsing #2#92

Merged
Enkidu93 merged 9 commits into
masterfrom
strict_parsing_#2
Sep 26, 2023
Merged

Strict parsing #2#92
Enkidu93 merged 9 commits into
masterfrom
strict_parsing_#2

Conversation

@Enkidu93

@Enkidu93 Enkidu93 commented Sep 25, 2023

Copy link
Copy Markdown
Collaborator

This change is Reviewable

@johnml1135

Copy link
Copy Markdown
Collaborator

The build is broken - let's fix that first.

@johnml1135

Copy link
Copy Markdown
Collaborator

src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs line 62 at r1 (raw file):

            try
            {
                useStrictParsing = bool.Parse((string?)buildOptionsObject?["useStrictParsing"] ?? "false");

So, strict parsing is false by default. Is that the desire as per Damien?

@Enkidu93

Copy link
Copy Markdown
Collaborator Author

The build is broken - let's fix that first.

That's because it requires the build_options update to Serval. It's not broken in and of itself. You can verify this locally.

@Enkidu93

Copy link
Copy Markdown
Collaborator Author

src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs line 62 at r1 (raw file):

            try
            {
                useStrictParsing = bool.Parse((string?)buildOptionsObject?["useStrictParsing"] ?? "false");

So, strict parsing is false by default. Is that the desire as per Damien?

As I understood it...See this connected comment.

@johnml1135 johnml1135 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

@johnml1135

Copy link
Copy Markdown
Collaborator

src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs line 62 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

As I understood it...See this connected comment.

ok. Makes sense. See comment in Serval about adding swagger docs about this.

@johnml1135 johnml1135 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 15 of 15 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@ddaspit ddaspit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 14 files at r1, 15 of 15 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

@Enkidu93 Enkidu93 merged commit 32eff04 into master Sep 26, 2023
@Enkidu93 Enkidu93 deleted the strict_parsing_#2 branch September 26, 2023 20:45
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