Add endpoint to fetch metadata about projects for onboarding script#3679
Add endpoint to fetch metadata about projects for onboarding script#3679
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3679 +/- ##
==========================================
- Coverage 81.91% 81.84% -0.08%
==========================================
Files 618 618
Lines 38568 38603 +35
Branches 6311 6290 -21
==========================================
Hits 31594 31594
- Misses 6000 6048 +48
+ Partials 974 961 -13 ☔ View full report in Codecov by Sentry. |
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 1 file and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Nateowami).
-- commits line 2 at r1:
Inserting a d will make enpoint more on point.
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 510 at r1 (raw file):
} SFProject project = _realtimeService
I understand that SFProject here should be SFProject? since a null value was returned.
I understand that an existing example in the code was also SFProject, but let's use SFProject? for this new code.
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 518 at r1 (raw file):
} object result = new
I'm used to seeing the RPC files call into service files for the implementation. I won't complain about that here, but I do think it will be cleaner if we don't start to let implementation creep into the RPC files.
f41b349 to
24315c4
Compare
def0ad1 to
4d3bb54
Compare
4d3bb54 to
4a92067
Compare
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 3 comments.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @marksvc).
Previously, marksvc wrote…
Inserting a
dwill make enpoint more on point.
Fixed. Thanks.
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 510 at r1 (raw file):
Previously, marksvc wrote…
I understand that
SFProjecthere should beSFProject?since a null value was returned.
I understand that an existing example in the code was alsoSFProject, but let's useSFProject?for this new code.
Done. However, I'm not sure we've actually improved anything by doing that. Is there a reason we haven't turned on the actual warnings for null references? There would be a few hundred warnings.
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 518 at r1 (raw file):
Previously, marksvc wrote…
I'm used to seeing the RPC files call into service files for the implementation. I won't complain about that here, but I do think it will be cleaner if we don't start to let implementation creep into the RPC files.
I know; we usually have a service class that is separate from the RPC controller. And while I appreciate the separation of concerns, very often for me it just obfuscates what each RPC method actually does. If I Ctrl+Click to view the implementation, it takes me to the interface. I have to right click and choose "Go to implementations". And then usually I lose context and have trouble jumping back. For things where the service needs more logic, it can make sense. But for this controller I thought it would be simpler to put everything in one place.
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 1 file and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami).
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 510 at r1 (raw file):
Previously, Nateowami wrote…
Done. However, I'm not sure we've actually improved anything by doing that. Is there a reason we haven't turned on the actual warnings for null references? There would be a few hundred warnings.
Hmm. Right. I know that at one time we needed to do transitions of code from not using nullable types, to using nullable types. Maybe this file isn't configured to require it. I made a note to look into it.
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 518 at r1 (raw file):
Previously, Nateowami wrote…
I know; we usually have a service class that is separate from the RPC controller. And while I appreciate the separation of concerns, very often for me it just obfuscates what each RPC method actually does. If I Ctrl+Click to view the implementation, it takes me to the interface. I have to right click and choose "Go to implementations". And then usually I lose context and have trouble jumping back. For things where the service needs more logic, it can make sense. But for this controller I thought it would be simpler to put everything in one place.
Okay. Yeah that is annoying about Ctrl+Click.
This change is