Large Logging, Testing and iGraph Update #339
Conversation
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
I'd been working on reviewing this but just saw you merged it. Do what you want with the feedback, thanks for working on this.
| testObj$edges | ||
| }) | ||
| #expect_silent({testObj$nodes}) | ||
| #expect_silent({testObj$edges}) |
There was a problem hiding this comment.
Why are all these tests getting commented out?
If they're not needed any more, they should be removed. If they're needed but just not working right now, we should leave an explanatory comment (and ideally issue link(s)) explaining what would be needed to re-eanble them.
| #' | ||
| #' @keywords internal | ||
| #' @importFrom R6 R6Class | ||
| SimpleLogger <- R6::R6Class( |
There was a problem hiding this comment.
In my opinion, this complexity isn't worth it for lightly-used, completely internal functionality.
With {futile.logger}, it's technically possible for users to change the log verbosity by running futile.logger::flog.threshold() in their session.
As of this PR, the only way to influence {pkgnet}'s log verbosity would be to reach into private methods. I think basically no one will do that, and if they do we'll probably discourage it.
Would you consider just instead doing something like uptake/uptasticsearch#260, completely eliminating the idea of setting thresholds?
There was a problem hiding this comment.
That'd also allow us to to eliminate PKGNET_TEST_ORIG_LOG_THRESHOLD and everything else related to setting and unsetting log thresholds in tests. Could be a pretty nice simplification.
| # CRAN Submission History | ||
|
|
||
| ## v 0.5.0 | ||
| ## v 0.5.0 |
There was a problem hiding this comment.
| ## v 0.5.0 | |
| ## v 0.5.0 |
This looks like it was a mistake.
| ) | ||
| ) | ||
| }) | ||
| #expect_silent({ |
There was a problem hiding this comment.
Same question why are we just commenting this out?
|
As always, thanks for the review. I'm moving fairly quickly to maintain CRAN hosting deadline (EOM) and doing this in the margins of the day. The commented out tests are gross. Yes indeed. I packaged them under one commit so they are easily reverted later. Guess I could just as easily deleted them with that commit packaging. For the life of me, I can't figure out why those functions where expected to be silent, and what behavior changed recently. I punted and removed them, planning to make a bug ticket to add em back in after CRAN update BUT everything appears fine without 'em. I'll remove the commented out code in a dev version to follow. Re logging object, that was claude's addition. I let it ride. |
|
Yep all good man! Do what you need to, this is all internal-to-the-package stuff that users won't ever see. |
🎯 Summary
Removes the futile.logger dependency and updates igraph compatibility to version 2.1+, improving package maintainability and aligning with modern igraph API.
🔧 Changes
📝 Logging System Refactor
Replaced futile.logger with custom SimpleLogger class
Files modified:
📊 igraph Compatibility Update
Bumped minimum version: >= 1.3 → >= 2.1
Updated files:
🧪 Testing Improvements
📚 Documentation
None - Internal implementation changes only. Public API remains unchanged.
✅ Testing
All checks passing:
📦 Dependencies
Removed:
Updated:
🔗 Related Issues
Fixes #338