Rename the 'marginal' attribute of MarginalModel to 'modified' #706
Conversation
…cludes a deprecation warning
Codecov Report
@@ Coverage Diff @@
## master #706 +/- ##
==========================================
+ Coverage 80.02% 80.05% +0.02%
==========================================
Files 38 38
Lines 2854 2858 +4
==========================================
+ Hits 2284 2288 +4
Misses 570 570
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This looks great! The one thing I was wondering is if you tested out the getproperty function you added for catching old calls to the "marginal" name. Maybe we should just keep one of the lines in the test file you updated as the old way, to make sure the warning appears as intended?
|
Ah yeah I had done manual testing by keeping one of the old lines and running the test suite. The line showed up as a properly formatted warning, but the tests still passed. If you want, I can put that back in, but I figured users would be confused if it showed up for them. |
|
The value of keeping it in would be to make sure that the new function you added gets tested properly, but I'm surprised to see that the coverage tests that ran say that 100% of the diff is hit... I would've thought that it would miss those lines... Maybe it's possible that the model tests (i.e. DICE and FUND) that get run by the test suite are hitting those lines of the function... Either way I think it would make sense to just leave one in the test file, and add a comment that says you are testing the soon-to-be-deprecated behavior |
|
Yea I agree, we've done that with some of the other deprecated code, just do leave a note and it'll be easy for us to find it later! |
… value is also returned
…cludes a deprecation warning
… value is also returned
…thub.com/arnavgautam/Mimi.jl into arnavgautam/renameMarginalModelAttribute
|
Updated it with the test, sorry for the git weirdness but I guess the VSCode interface messed me up |
|
@lrennels do you know why some of your most recent commits are showing up here in the diffs of the "Files changed" even though they've already been commited to master? I mean I think it's fine to just merge this I'm just confused why they are showing up as different against master if they are already on master... |
|
@ckingdon95 Oh that's strange no I don't know, I was thinking maybe if I approved it would re-compare to master but it didn't. Odd ... |
|
@ckingdon95 I just merged a little toml update so I'll update this branch and see if that helps ... looks like it did! So I think somehow the comparison just didn't get redone. Shall I merge? |
|
awesome thanks Lisa! |
Renames the attribute, and adds an alias for the old 'marginal' name which includes a deprecation warning
See #530