-
Notifications
You must be signed in to change notification settings - Fork 7
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
DUI3-126 shared files #12
DUI3-126 shared files #12
Conversation
- And other simplications, removing dup code etc.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #12 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 161 161
Lines 3155 3155
Branches 340 340
=====================================
Misses 3153 3153
Partials 2 2 ☔ View full report in Codecov by Sentry. |
@@ -349,7 +343,7 @@ public async Task Send(string modelCardId) | |||
var result = await unitOfWork.Service | |||
.Execute( | |||
mapMembers, | |||
sendInfo, | |||
modelCard.GetSendInfo("ArcGIS"), // POC: get host app name from settings? |
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.
okay we will need to resolve this at one point :D (just a comment)
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.
yes, I'm fairly certain we have a ticket for this, but it was always low prio
var sendResult = await sendOperation.Service | ||
.Execute( | ||
revitObjects, | ||
sendInfo, | ||
modelCard.GetSendInfo(_revitSettings.HostSlug.NotNull()), |
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.
the pattern does seem to be there for revit?
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.
partly yes, but still this HostSlug in settings depends on old kit classes which we will change it.
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.
minor comments, feel free to merge without acting on any of them!
@@ -15,6 +15,7 @@ public static void AddConnectorUtils(this SpeckleContainerBuilder builder) | |||
// send operation and dependencies | |||
builder.AddSingleton<CancellationManager>(); | |||
builder.AddScoped<ReceiveOperation>(); | |||
builder.AddScoped<AccountService>(); |
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.
from usage, to me it looks like this dude could be better as a singleton?
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.
(not essential)
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.
yea, doing it
|
||
namespace Speckle.Connectors.Utils.Operations; | ||
|
||
public class AccountService |
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.
please add comment on how this class is expected to be injected around, to prevent fuckups like we had in the past when we changed registration and everything went haywire!
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.
you are god damn right. doing it
ReceiveInfo
record as we did before withSendInfo
Corresponding PR on UI side -> specklesystems/speckle-server#2459