Add method table as key for method_info
#140
Conversation
timholy
left a comment
timholy
left a comment
There was a problem hiding this comment.
This looks great and less intrusive than expected. I don't have any reservations about the implementation.
It probably makes sense to bump CodeTracking's version to 2.0.0 as part of this PR. You could also make it 2.0.0-DEV if you want to signal that other breaking changes should land before release.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #140 +/- ##
===========================================
- Coverage 89.50% 69.60% -19.90%
===========================================
Files 3 3
Lines 381 408 +27
===========================================
- Hits 341 284 -57
- Misses 40 124 +84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
As all other manipulation functions for IdDict don't convert, it's best to be consistent and catch errors when we detect a mismatch.
serenity4
force-pushed
the
parametrize-by-mt
branch
from
cb638fa to
9f354c6
Compare
serenity4
force-pushed
the
parametrize-by-mt
branch
from
9f354c6 to
dc208c1
Compare
|
Is it ok to drop 1.6 and support only from 1.10 onwards? |
Definitely! |
Tests will fail, that's normal
|
(forgot to squash before merge, I cleaned things up in 8e1ca1e) |
2 participants
Modify the format of
method_infoto have it keyed bymt::Union{Nothing, MethodTable} => sigpairs instead ofsig.This allows Revise and other packages to lookup
method_infoscoped by an external method table, if any (a value ofnothingrepresenting the internal method table).An alternative design would be use two dictionaries, where the current
sig => [(lnn, ex)...]mapping would be a value of an outermt => infomapping. I went with keeping a single dictionary (but modifying its key format) for simplicity, but I am open to other designs.