Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1198 +/- ##
==========================================
- Coverage 44.73% 44.71% -0.02%
==========================================
Files 266 266
Lines 8068 8077 +9
Branches 528 528
==========================================
+ Hits 3609 3612 +3
- Misses 4043 4049 +6
Partials 416 416
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
|
.NET 5 has an improved JSON Serializer: #1200 This would allow us to avoid some limitations with the current serializer: |
jasonleenaylor
left a comment
There was a problem hiding this comment.
Reviewed 10 of 13 files at r3.
Reviewable status: 10 of 13 files reviewed, all discussions resolved (waiting on @imnasnainaec)
docs/user_guide/docs/licenses/backend_licenses.txt, line 92 at r3 (raw file):
Version:2.1.0
Hmm, this is confusing a couple of ways.
- Just above we are including the license for version 2.2.0 Given my experience with .Net assembly references we almost certainly aren't using both.
- I guess the package versions here don't actually match the asp.net version? So these new ones presumably appeared from the use of
System.Text.Json.Serialization?
|
docs/user_guide/docs/licenses/backend_licenses.txt, line 92 at r3 (raw file): Previously, jasonleenaylor (Jason Naylor) wrote…
We're including |
|
Backend/Controllers/UserController.cs, line 41 at r5 (raw file):
Do we need to add 404 here now? |
jasonleenaylor
left a comment
There was a problem hiding this comment.
Reviewed 5 of 16 files at r4.
Reviewable status: 14 of 27 files reviewed, all discussions resolved (waiting on @imnasnainaec and @jasonleenaylor)
docs/user_guide/docs/licenses/backend_licenses.txt, line 92 at r3 (raw file):
Previously, johnthagen wrote…
We're including
--include-transitivein thedotnet-project-licensescommand, so that might have something to do with it? Due to how the dependencies are described in C# packages, it might be difficult for a license checker looking from the side to know how the final linking happens. Perhaps that is why there is duplication?
Perhaps, and the change to 5.0 didn't really change this. 🤷♂️
|
Backend/Controllers/UserController.cs, line 41 at r5 (raw file): Previously, jasonleenaylor (Jason Naylor) wrote…
Per discussions with @imnasnainaec documenting non-200 responses is of pretty low value because it doesn't change the generated type script code. It ends up being a bunch of bookkeeping without any real value. @imnasnainaec Please correct me if I'm mischaracterizing our conclusion about this. |
|
Some LGTM logs: |
|
@jasonleenaylor If we decide to accept this PR, the following administrative steps will need to be performed:
|
|
Edit: For now we decided to stick to the defaults. We may want to include some of the additional queries that CodeQL supports, but this does risk having more false positives:
|
jasonleenaylor
left a comment
There was a problem hiding this comment.
Reviewed 1 of 13 files at r3, 10 of 16 files at r4, 2 of 2 files at r7, 1 of 1 files at r8.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)
jasonleenaylor
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9.
Reviewable status:
For debugging after #1198.
Closes #1200
Closes #1149
is not nullsyntaxAddControllersWithViews(), which is not used by The Combine, as recommended for ASP.NET Core 3+ (https://stackoverflow.com/questions/62251347)System.Text.Jsonfor all JSON serialization into and out of API endpoints (default for ASP.NET Core 3.0+) (Migrate away from deprecated Newtonsoft JSON serializer #1149)get/set) to enable proper discovery during OpenAPI schema generationThis change is