-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix igraph deprecation warnings as well as a bug in issue network construction #264
Conversation
Signed-off-by: Maximilian Löffler <[email protected]>
The bug, described in Issue#260, practially limits the amount of edges that can be present in an issue network to 17 and additionally leads to warnings in every case where there are not exactly 17 edges present. It is caused by an incorrect parameter to the 'base::split' function. We intend to pass a vector containing the numbers 1 to nrow(add.links), but instead pass 1 to 17. Signed-off-by: Maximilian Löffler <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #264 +/- ##
==========================================
+ Coverage 79.89% 80.24% +0.34%
==========================================
Files 16 16
Lines 4905 4905
==========================================
+ Hits 3919 3936 +17
+ Misses 986 969 -17 ☔ View full report in Codecov by Sentry. |
In practice, GitHub is by far the most used data source for issues. By adjusting the default to 'github' only, we can greatly reduce the amount of raised warnings in production. Signed-off-by: Maximilian Löffler <[email protected]>
Signed-off-by: Maximilian Löffler <[email protected]>
I've identified the reason for why the pipeline fails for R 3.6 now: Package Independent of that, before we fix that issue, can we somehow enable the generation of the coverage report in our CI pipeline? Maybe it would be a good idea to generate the coverage report if the tests for the version, for which we generate the report, have finished successfully, even if the test pipeline fails for other versions? Just to avoid that the test coverage is not generated just because of specific issues in a single R version. What do you think @MaLoefUDS? |
That should be straightforward. I will look into it tomorrow 👍 |
I don't like to have green check marks for failing steps. That'd be weird. |
Is that something you want me to do inside of this PR? 🤓 |
We have two options: Either we open a new PR that fixes the CI pipeline, and rebase this PR after the other one is merged. Regarding how and when to update the |
I agree with you: since we are addressing deprecated features in here, I think it is legitimate to fix the CI here as well 👍 |
Regarding the workarounds in the install script: What of the two workarounds are only present for R 3.6? As I see it, both of the workarounds may come to play also for newer but strangely configured or incorrectly installed R versions. Context: Lines 67 to 80 in 24005e4
|
Actually non of the ones in your quoted excerpt. The workaround is only present on the Lines 81 to 83 in a87ff24
The other two version checks in your quoted excerpt are independent of R 3.6. The first one is because we want to support/recommend a minimum version number of |
Oh okay, I see 🤓 |
Deprecating 3.6 was decided in PR se-sic#264 because a subpackage of testthat now depends on R versions above 4.0. Signed-off-by: Maximilian Löffler <[email protected]>
Deprecating 3.6 was decided in PR se-sic#264 because a subpackage of testthat now depends on R versions above 4.0. Signed-off-by: Maximilian Löffler <[email protected]>
Deprecating 3.6 was decided in PR se-sic#264 because a subpackage of testthat now depends on R versions above 4.0. Signed-off-by: Maximilian Löffler <[email protected]>
Signed-off-by: Maximilian Löffler <[email protected]>
Interesting: The upload of the coverage report worked again without changing anything, right? |
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.
Thanks for this PR @MaLoefUDS. It looks really great, thanks for your efforts on changing all the reading and splitting tests due to the new default value of the issue source.
I spotted two minor issues:
- the new default value is not updated in the
README.md
yet (see details below) - Codecov points us to some uncovered functionality. We should take advantage of this and add a few tests for some of the uncovered functions revealed by Codecov. But definitively not for all of them. I don't care about everything in
util-bulk.R
, and we don't have a proper testing setup forutil-plot.R
yet. So we can ignore Codecov's complaints regarding these files. But Codecov also revealed a small number of functions in well-tested files such asutil-networks.R
that seem to be untested still, and I would like to add some small tests for them. Please see my comments below for the particular functions I am referring to.
expect_true(igraph::are.connected(simplify.network(g), "A", "Base_Feature")) # specific edge | ||
expect_true(igraph::are_adjacent(simplify.network(g), "A", "Base_Feature")) # specific edge |
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.
That's an interesting change, ending up in a completely different function name that has nothing in common with the previous name...
(Just a comment, nothing to worry about.)
Signed-off-by: Maximilian Löffler <[email protected]>
@MaLoefUDS Do you have any idea why the Coverage report in this issue is not updated? |
I do not know why the comment does not update. The docs state that the default behavior is to update the comment and otherwise send a new message. We did not update the default and there is no new comment, so we can assume that the codecov update happened and failed for some reason. We can force codecov to send new comments on every change to the PR, maybe that would help, but im unsure. Apart from that I have no idea 🤷♂️ |
Right now the comment from Codecov is up to date again and refers to the most recent commit (see quote below). So I guess something got stuck there (for whatever reason). So let's assume it works well.
|
I don't think that @hechtlC would agree with that 😉 We already receive lots of e-mails, we don't need to receive multiple e-mails for one push event... If codecov is able to properly update the first comment (as it seems to be, at the moment), updating the comment is preferred over sending new comments, I guess. |
Signed-off-by: Maximilian Löffler <[email protected]>
Signed-off-by: Maximilian Löffler <[email protected]>
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.
Everything looks good to me now. So, the only thing that is missing is to add the changes to the NEWS.md
.
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.
Looks good to me! Thanks @MaLoefUDS .
Signed-off-by: Maximilian Löffler <[email protected]>
Deprecating 3.6 was decided in PR se-sic#264 because a subpackage of testthat now depends on R versions above 4.0. Signed-off-by: Maximilian Löffler <[email protected]>
Prerequisites
showcase.R
with respect to my changes.dev
.Description
Changelog