Importer Rework - Fix Warnings #3877
Conversation
|
I have fixed all of the "obvious" stuff an will try to highlight and @ people here to get feedback on the few remaining things that I am not certain about in an effort to not introduce new bugs. The last person that made the change I am concerned about, which will not always be the correct person to ask, I apologize in advance. @tpurschke in Report.razor:636 complianceReport.ShowAnyRule Parameter is assigned a value outside of its intended scope which Blazor flags with a warning. I have some rough idea how to fix this after some googling but Id love to either do a seperate pull request with an extensive review of this or a meeting to quick talk and implement these changes Similarly in DeviceReportController.cs:59 the method ContainsRules() is either intentionally meant to hide DeviceReport.ContainsRules() ... or not. A quick "yes this was intentional" would be enough then I can make it so :) @abarz722 In ComplianceChecks.razor:106 a new ComplianceCheck is called and its apiConnection set to null, which is not allowed according to the nullable value. I've tried scanning through the related documents to see if making it nullable is an option that can be done quickly and conclude that it does not seem very viable. Is there any way to sensibly change this line to avoid the null assignment? |
|
regarding ComplianceChecks.razor: As the apiConnection seems not to be necessary, maybe the called method (and underlying methods) can be made static. Currently @Robin-Smets is a bit deeper in this sources. |
same answer as from Achim: the compliance report stuff is from Robin, so please reach out to him.
the Controller class is the attempt to move all the logic to this class, so ContainsRules should eventually only exist in DeviceReportController. Currently both versions are dummy methods always returning true, so they must be implemented (not in your PR). So my suggestion is to remove DeviceReport.ContainsRules() and only work with DeviceReportController.ContainsRules() if possible |
|
@Elutrixx pls. update branch |
|
let us leave this PR until next week to make sure we do not interfere with customer upgrades this week |
tpurschke
requested review from
tpurschke
and removed request for
Robin-Smets
tpurschke
left a comment
tpurschke
left a comment
There was a problem hiding this comment.
nice - just a view remarks - see inline
let's discuss this on Monday
| } | ||
|
|
||
| } | ||
| (nextPosition, nextParent, lastPosition) = PrepareOrderNumberCreation(currentQueueItem, lastPosition, nextQueueItem); |
There was a problem hiding this comment.
@Robin-Smets can you review this part please? (no hurry, just add to your todo list)
| private ReportBase? currentReport; | ||
|
|
||
| private FWO.Ui.Shared.TabSet rsbTabset; | ||
|
|
There was a problem hiding this comment.
@Robin-Smets same here for line 636 (see copilot remark regarding test install)
|
| } | ||
|
|
||
| //TODO: Does this need to be async? | ||
| protected virtual async Task SetComplianceDataForRule(Rule rule, ApiConnection apiConnection, Func<ComplianceViolation, string>? formatter = null) |
There was a problem hiding this comment.
@Robin-Smets Is this purposfully left as async? Directing this question at you as you took the awaited method out of the code block, but maybe there is a different reason this needs to be run asynchronously
|
Updated the fixed warnings, apart from open conversations all warnings are fixed (excluding the two warnings that Robin wanted to fix by himself) |
3 participants
Goal is to resolve all build warnings in the importer rework branch to enable the 'fail on warning' to be enabled again and would resolve #3865 .