-
Notifications
You must be signed in to change notification settings - Fork 43
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
Switches the app to a single process #779
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #779 +/- ##
==========================================
+ Coverage 62.49% 63.36% +0.86%
==========================================
Files 611 585 -26
Lines 18564 17863 -701
Branches 1305 1254 -51
==========================================
- Hits 11602 11319 -283
+ Misses 6620 6204 -416
+ Partials 342 340 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
About to start reviewing the App side. I reviewed the library side, since nobody has done that prior. Requested changes on the library side have been submitted as 'issues' on the library repo: CC. @Greg-Nexus Can you set up labels over there? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues on the App side, all's good.
All requested changes and PR related feedback concern the library side itself. These can be found under the issues section in the library repo.
In general, I'm a fan of the new way to create verbs, and it's really surprising how clean this works (the command handler doesn't need to know anything about the fact it's talking over the network at all!).
Though, at the same time, I have reservations about the two-process design (having an idling client which does nothing), and its impact on startup time and RAM usage. Relevant details are covered in library issues above.
Misc Note: FOMOD CLI installation is also now dead. This wasn't mentioned anywhere, but figured I might aswell.
Although I still have very heavy reservations regarding the client process sitting in RAM, doing nothing, the App side can be merged. Library side can be fixed at a later time. PS. (I'm down to do it, since I'm familiar with said code now too, as I had to review it) |
This is a rewrite of the CLI portion of the app to support a single main process and "client" CLI processes that communicate over TCP localhost connections.
NexusMods.SingleProcess
github projectThis change also implements a new CLI verb registration system:
[Injected]
(filled from DI) or[Option(...)]
which are considered CLI options.AddVerb(() => MethodName)
Not handled in this PR and will need to be handled in the future: