-
Notifications
You must be signed in to change notification settings - Fork 9
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
Feature/102 fix the sonarcloud issues in iforwardsover #104
Feature/102 fix the sonarcloud issues in iforwardsover #104
Conversation
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.
This update does not remove build warning for Demo07pMCInversion: The event Demo07PmcInversion.PmcForwardSolver.PropertyChanged is never used. Will that be done in another update?
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.
I just updated the part that was affected by the IForwardSolver changes, do you think we should fix this also?
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.
I fixed this and a few other code quality suggestions in the file.
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.
HUGE effort on the reorganization of the IForwardSolver. Thank you!
…t PropertyChanged was not being used
@dcuccia @hayakawa16 I plan to merge this PR tomorrow as it has been open for 2 weeks. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
I'll look at it tonight |
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.
@lmalenfant can you summarize what we did on this ticket? Was it just re-organizing the order of functions and fixing comments? (E.g. no changes to method signatures/API?) If we changed the API, a) how did we choose to augment/remove methods, and b) have we tested the "loosely types" Matlab calls??
Yes exactly, no other changes. |
Thanks. Please go ahead with the merge. I did see the removal of implicit "target typed" constructor calls in the solver demos. In my opinion, those are unnecessary additions that add verbosity (and decrease conciseness), but that's just a preference based on my experience with functional style programming. |
I completely agree with you for cases where the type is obvious but this in this code it is not obvious, this is the reasoning behind the change: Since these scripts are for teaching purposes, I thought it would be good to put them back in. I tend to follow Resharper's advice because this type of change is their "Raison d’être" :) |
@hayakawa16 before I add reviewers to the PR, I would like to review the XML comments with you first, I think we should remove the comment that states "Overload of x function" because the fact that there are multiple method definitions, means that they are overloads so this comment seems redundant, what are your thoughts?