Skip to content
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

Merged
merged 13 commits into from
Jul 17, 2024
Merged

Deal with normalization collisions in reimport #532

merged 13 commits into from
Jul 17, 2024

Conversation

palday
Copy link
Collaborator

@palday palday commented Jul 16, 2024

  • give more informative error messages
  • allow customization of normalization
  • fix a few small things while hitting that code

closes #418

* 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
@palday palday requested a review from ararslan July 16, 2024 19:18
src/namespaces.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.15%. Comparing base (62ae259) to head (52d80f8).

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.
📢 Have feedback on the report? Share it here.

@palday palday requested a review from ararslan July 16, 2024 22:39
Copy link
Member

@ararslan ararslan left a 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.

@palday
Copy link
Collaborator Author

palday commented Jul 17, 2024

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. 😄

@palday
Copy link
Collaborator Author

palday commented Jul 17, 2024

HUH, so it turns out that the exports are order sensitive ?! That ordering is lost when we do do collect(keys(...)), so everything breaks.

@palday palday merged commit 0bb0d53 into master Jul 17, 2024
14 of 17 checks passed
@palday palday deleted the pa/rlibrary branch July 17, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERROR: invalid redefinition of constant with R library
2 participants