Add length check guards for delete methods

[gaurav-arya]

This fixed #13 for me. It seems like a safe solution, since there's no harm in deleting all existing methods that may be around as they will all be redefined subsequently -- but let me know if you foresee any issues.

Update; the PR now just adds length check guards, which should be an improvement over the previous behaviour although an issue should be created for the assumption on index 1 and 2

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e1124d9) 97.50% compared to head (16e6873) 97.50%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #31   +/-   ##
=======================================
  Coverage   97.50%   97.50%           
=======================================
  Files           2        2           
  Lines         120      120           
=======================================
  Hits          117      117           
  Misses          3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Tortar]

maybe a possible problem is that it deletes also the user-defined ones if I'm not mistaken, which can be an unwanted side effect in some cases

[gaurav-arya]

Hm. Maybe the safest solution is to store the method objects that we create in thread-safe global storage, check if they still exist upon redefinition, and if so delete them? (To also avoid relying on any assumed orderings of collect(methods)

[gaurav-arya]

I hope there's a simpler solution though 🙂 for this PR it could probably already be an improvement to just guard the original delete method calls with checks that they exist, but keep the assumption about the ordering

[Tortar]

Also your second solution seems like a good one to me, even if the first is safer. So I would say: let's go with it in this pr :-)

[Tortar]

Maybe we can create an issue saying that relying on the ordering of the methods table is not good practice, and so it should be changed in the future?

[Tortar]

I approve, but I don't have merge privileges so we need to wait for merging it

[gaurav-arya]

I'd like to hold this PR for a little bit to investigate the behaviour with Revise some more. Although it stops the original error, I think I ran into an unexpected methods state while using this branch with Revise in regular local development. (And of course if either you have the time for a little local Revise testing that would also be appreciated!)

[BeastyBlacksmith]

I merged this, since irrespective of Revise this fixes the 0-field to non 0-field struct error, I put in the tests