Merge changes in the development branch #53

onurulgen · 2021-08-05T12:38:28Z

No description provided.

…CommonUtils::CalculateCentreOfGravity

…gApp into development

…rom text file

When a PR is created and for each commit of a PR, it will generate static code analysis by using CppCheck and upload the results as a comment

For each commit or PR, it will run tests, and code coverage. It will also upload code coverage results to codecov.io, which can be followed at https://app.codecov.io/gh/CemrgDevelopers/CemrgApp

Add more tests

github-actions · 2021-08-05T12:39:10Z

✅ Code Analysis Results - no issues found! ✅

ericspod · 2021-08-05T15:33:38Z

Do we know the issue with the tests? Also looking at the list of code analysis errors many are pure stylistic or about if-conditions related to the verbose code. Duplicate if-conditions can be condensed to one block but there's a number of things flagged that aren't duplicates so these checks maybe should be disabled.

onurulgen · 2021-08-05T15:53:02Z

Do we know the issue with the tests? Also looking at the list of code analysis errors many are pure stylistic or about if-conditions related to the verbose code. Duplicate if-conditions can be condensed to one block but there's a number of things flagged that aren't duplicates so these checks maybe should be disabled.

No error with the tests, it just can't find the macro definition even though it can find the previous macro definition in the CemrgCommandLineTest.cpp. That's weird for sure.

I can disable the style checking if it's requested.

alonsoJASL · 2021-08-05T16:16:35Z

I'm still sifting through the errors in #53 (comment) . But I can say right now that anytime you find a

if(verbose)
    MITK_INFO << "message";

You can change it for

MITK_INFO(verbose) << "message";

that should get rid of a lot of those messages. I'll check the others and comment on them.

ericspod · 2021-08-05T16:46:19Z

The test job does indicate it failed though, is that not a concern?

codecov-commenter · 2021-08-11T12:33:20Z

Codecov Report

❗ No coverage uploaded for pull request base (master@73f9973). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master     #53   +/-   ##
========================================
  Coverage          ?   9.70%           
========================================
  Files             ?      72           
  Lines             ?   12078           
  Branches          ?       0           
========================================
  Hits              ?    1172           
  Misses            ?   10906           
  Partials          ?       0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73f9973...2b050da. Read the comment docs.

onurulgen · 2021-08-13T15:23:24Z

Now, I've fixed most of the code analysis problems, only a few important ones are remaining. Please look into them.

ericspod · 2021-08-16T04:46:24Z

Many of the code issues above relate to some variable set a static value then used in a condition, I guess these are placeholder for setting the values from a GUI element? Others are about variable shadowing so the variable being flagged should be renamed. Those relating to unsafe types I'm not sure exactly what to do, those that don't have constructors but should probably do need constructors. Other things like using prefix ++/-- are clear about what to change.

CemrgApp/CemrgApp/Modules/CemrgAppModule/src/CemrgAtriaClipper.cpp Lines 444 the complaint is about 1.0 and 1.5 being cast to unsigned long which 1 in both cases, this should be changed to just be one if that was the intent of the ternary operator or changed to not cast to unsigned long.

…nTrueFalses

alonsoJASL

All looks good 👍

Thanks @onurulgen for an amazing work.

OrodRazeghi

@alonsoJASL almost everything looks good, only a few comments for you to have a look. Basically:

We need to create new issues for some of the topics mentioned in these comments, so we don't forget to come back to them in future.
Also, we have duplication of code in the cmdapps, which makes keeping track of changes in plugins and matching them with cmdapps difficult. I think we need a new issue for this too, to think about solutions in future.

.github/workflows/build.yml

CemrgApp/Applications/MainApp/version.txt

CemrgApp/Modules/CemrgAppModule/src/CemrgAtriaClipper.cpp

CemrgApp/Modules/CemrgAppModule/src/CemrgScar3D.cpp

OrodRazeghi · 2021-10-19T13:57:52Z

CemrgApp/Modules/CemrgAppModule/test/CemrgTestCommon.cpp

-}
-
-#endif // kcl_cemrgapp_renderwindoweditor_Activator_h
+constexpr decltype(CemrgTestData::surfacePaths) CemrgTestData::surfacePaths;


No sure what's happening here, is renderwindoweditor removed?

Created #55

For some reason, github is taking this CemrgTestCommon.cpp and comparing it to the recently deleted kcl_cemrgapp_renderwindoweditor_Activator.h . I don't know why github decided to do that.

OrodRazeghi · 2021-10-19T13:59:58Z

CemrgApp/Modules/CemrgAppModule/test/test.hpp

-  {
-    BERRY_REGISTER_EXTENSION_CLASS(QmitkCemrgRenderWindowEditor, context)
-  }
+    Q_OBJECT


same as the other comment about the renderwindoweditor

Same as the other comment. For some reason, github caught me deleting the kcl_cemrgapp_renderwindoweditor_Activator.h and put that change on this test.hpp.

Not sure why, as they're in very different paths! Do you have any idea about this, @onurulgen ?

For context, I deleted the renderwindow plugin we had in CemrgApp (commit 100fd8), but for some reason, github caught that deletion as a change in this test.hpp file

CemrgApp/Plugins/kcl.cemrgapp.wathca/src/internal/WallThicknessCalculationsView.cpp

alonsoJASL

Thanks @OrodRazeghi for your feedback. I think we can merge this one for now. I created #54, #55, and #57 for the things that need to be addressed but not critically.

alonsoJASL and others added 30 commits April 27, 2021 12:36

[werror-unused-result-fix] checking return values for fscanf in Cemrg…

1c12a73

…CommonUtils::CalculateCentreOfGravity

[werror-unused-result-fix] fixed type in MITK_INFOs in lines 839 and 857

d568a11

[CICD/bugfixes] Compilation warnings/errors fixed.

441970b

Merge branch 'development' of https://github.com/CemrgDevelopers/Cemr…

d000247

…gApp into development

[feature] added CemrgCommonUtils: SetSegmentationEdgesToZero, FillHoles

ba381c3

[feature][cmdapp] ScarProjection Options created

8c319e4

[bugfix] SetSegmentationEdgesToZero fix

93bf4e5

Merge branch 'development' into feature/cmd-scarmap-projection-opts

dfbe98c

Fix compilation errors

8c496fe

MitkCemrgScarProjectionOptions - finished

f70bf2b

Merge branch 'development' into feature/cmd-scarmap-projection-opts

ffefeb9

Ditch GetDirectorySeparator calls since Windows recognises slashes

1a31150

Fix MLib and M3DLib paths for macOS

13e93ec

[minor] remove call to mitk::IOUtil::GetDirectorySeparator in cmdapp

36c0918

Add automatic building and deploying of CemrgApp by using Github Actions

8f28d8c

Fix branch for Build & Deploy

abcaedb

[feature] Splash screen with functionality to read cermgapp version f…

0ed5467

…rom text file

[feature] nicer splashscreen with functionality and disclaimers

c6e358b

[feature] Splashscreen v1 - finished

863db4d

Reduce clutter by adding Points and Point types to CemrgMeasure

73234ee

Ditch docker for the sake of MIRTK libraries

1a679db

Setup tests and add test for CemrgMeasure

28e594c

Fix compilation errors in the test of CemrgMeasure

5af1890

Fix build errors of tests

c15c3d7

Fix Build & Deploy Github Action to run if a release is published

fb7a1fd

Add Github Action for static code analysis

36d6657

When a PR is created and for each commit of a PR, it will generate static code analysis by using CppCheck and upload the results as a comment

Add Github Actions for Tests and Code Coverage

29ea63b

For each commit or PR, it will run tests, and code coverage. It will also upload code coverage results to codecov.io, which can be followed at https://app.codecov.io/gh/CemrgDevelopers/CemrgApp

Small fixes for test template

c0f6f67

Add a common class for facilitating sharing among tests

b6f753f

Modify CemrgMeasureTest to enable using of the common class

3853645

Merge pull request #52 from CemrgDevelopers/feature/tests

954c4a0

Add more tests

onurulgen requested review from OrodRazeghi, alonsoJASL and ericspod August 5, 2021 12:38

alonsoJASL added 3 commits August 5, 2021 18:06

[style-fix] fixed if(verbose) MITK_INFO statements.

a4d61d6

[style-fix] CemrgCommandLine added debug variable and style changes

db1f4e4

[style-fix] CermgCommonUtils variables out of scope fixed

e038511

Fix static code analysis errors

d3a9766

alonsoJASL added 3 commits October 18, 2021 15:39

[style-fixes] ShadowDeclarations, knownunReadVariables, knownConditio…

89cc398

…nTrueFalses

[style-fix] Added constructors for plugins.

aeb009f

[style-fix] constructors, knownConditionsTrueFalse, unUsedVariables

c92eb03

alonsoJASL removed the request for review from ericspod October 18, 2021 18:13

alonsoJASL added 2 commits October 18, 2021 19:42

[style-fix] YZSegView shadowed declarations

5703688

[style-fix] Removed kcl.cemrgapp.renderwindow - no longer needed

100fd84

alonsoJASL previously approved these changes Oct 19, 2021

View reviewed changes

OrodRazeghi reviewed Oct 19, 2021

View reviewed changes

[doc] Added comprehensive description of commented placeholder variables

3e72b14

alonsoJASL dismissed their stale review via 3e72b14 October 20, 2021 11:29

[style-fix] minor - CemrgAtriaClipper binary ball radius set to 1

2b050da

OrodRazeghi approved these changes Oct 20, 2021

View reviewed changes

alonsoJASL approved these changes Oct 20, 2021

View reviewed changes

OrodRazeghi merged commit 69e5e1e into master Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge changes in the development branch #53

Merge changes in the development branch #53

onurulgen commented Aug 5, 2021

github-actions bot commented Aug 5, 2021 •

edited

Loading

ericspod commented Aug 5, 2021

onurulgen commented Aug 5, 2021

alonsoJASL commented Aug 5, 2021

ericspod commented Aug 5, 2021

codecov-commenter commented Aug 11, 2021 •

edited

Loading

onurulgen commented Aug 13, 2021

ericspod commented Aug 16, 2021

alonsoJASL left a comment

OrodRazeghi left a comment

OrodRazeghi Oct 19, 2021

alonsoJASL Oct 19, 2021

alonsoJASL Oct 20, 2021

OrodRazeghi Oct 19, 2021

alonsoJASL Oct 19, 2021

alonsoJASL Oct 20, 2021

alonsoJASL left a comment

Merge changes in the development branch #53

Merge changes in the development branch #53

Conversation

onurulgen commented Aug 5, 2021

github-actions bot commented Aug 5, 2021 • edited Loading

✅ Code Analysis Results - no issues found! ✅

ericspod commented Aug 5, 2021

onurulgen commented Aug 5, 2021

alonsoJASL commented Aug 5, 2021

ericspod commented Aug 5, 2021

codecov-commenter commented Aug 11, 2021 • edited Loading

Codecov Report

onurulgen commented Aug 13, 2021

ericspod commented Aug 16, 2021

alonsoJASL left a comment

Choose a reason for hiding this comment

OrodRazeghi left a comment

Choose a reason for hiding this comment

OrodRazeghi Oct 19, 2021

Choose a reason for hiding this comment

alonsoJASL Oct 19, 2021

Choose a reason for hiding this comment

alonsoJASL Oct 20, 2021

Choose a reason for hiding this comment

OrodRazeghi Oct 19, 2021

Choose a reason for hiding this comment

alonsoJASL Oct 19, 2021

Choose a reason for hiding this comment

alonsoJASL Oct 20, 2021

Choose a reason for hiding this comment

alonsoJASL left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 5, 2021 •

edited

Loading

codecov-commenter commented Aug 11, 2021 •

edited

Loading