-
Notifications
You must be signed in to change notification settings - Fork 59
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
Deal with normalization collisions in reimport #532
Conversation
* detect name collisions and give a nice warning message * use gensym instead of a default name to avoid collisions of hidden module * allow custom normalization
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #532 +/- ##
==========================================
+ Coverage 77.49% 80.15% +2.65%
==========================================
Files 26 26
Lines 1684 1693 +9
==========================================
+ Hits 1305 1357 +52
+ Misses 379 336 -43 ☔ View full report in Codecov by Sentry. |
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.
countmap
is such a simple function that I almost feel like it could be more convenient to basically inline it rather than load StatsBase (assuming it isn't already a transitive dependency, which it probably is). Or perhaps separate this chunk out into something like
function normalized_names(members, replacements...)
occurrences = Dict{Symbol,Int}()
for member in members
x = Symbol(replace(member, replacements...))
occurrences[x] = get(occurrences, x, 0) + 1
end
duplicated = [k for (k, v) in occurrences if v > 1]
if !isempty(duplicated)
throw(ArgumentError("Names are not unique after normalization: " * join(duplicated, ", "))
end
return collect(keys(occurrences))
end
But anyway, doesn't matter to me, do what feels right.
It is already a transitive dependency (StatsModels), which is why I decided to just grab it that way. But I like your minor refactor, so I'll update to go that way. 😄 |
HUH, so it turns out that the exports are order sensitive ?! That ordering is lost when we do do |
closes #418