Various code cleanups and modernisations#36
Various code cleanups and modernisations#36andydotxyz merged 14 commits intofynelabs:mainfrom Jacalz:cleanups
Conversation
|
It looks like the CI is not matching the versions required in this update, so I guess it also needs to be changed to match what Fyne is using upstream? |
|
Good point. Should be fixed now |
|
Ping. Would you mind doing a re-review of this? :) |
andydotxyz
left a comment
There was a problem hiding this comment.
Sorry this dropped off my radar - looks good thanks
|
I had forgot that the Go update made changes to comment formatting. I have now re-formatted the code 👍 |
|
Any idea what the failure means @Jacalz ? |
|
It was missing some package. Sorry about that. Should be working now 😅 |
|
Hmm, the changes don't seem to have fixed the static-check failure. I don't understand the message, do you? |
|
It is a very strange error. From what I can tell, the test fails when testing with coverage data enabled because it tries to compare the binaries and they no longer match (due to different coverage?). I added a change to skip the tests when we get to that point. We can probably update it in the future to not run it on the |
|
Can this skips really have dropped this 20% in coverage? |
|
Seems very strange. I tried to rectify it by not calling |
|
Hmm, no change with this new approach @Jacalz. Oh I see what has happened - it added the cmd folder compare to previous test coverage. |
andydotxyz
left a comment
There was a problem hiding this comment.
Commented on the thread - seems the coverage issue was not that test after all, do you want to go back to the previous approach, or leave as is?
|
Sorry for the late reply. I tidied up the approach (the first approach skipped the whole test and was never a good one) a bit to make sure that the tests only are ended at that point when the tests are including coverage information. Should be ready now finally :) |
Just a small tidy to keep code quality up and in sync with Fyne v2.5.0.