Skip to content

Conversation

rjplevin
Copy link
Collaborator

@rjplevin rjplevin commented Mar 5, 2019

  • Eliminated global dict of component defs
  • Fixed uses of eval to avoid precompilation issues

rjplevin added 2 commits March 5, 2019 13:40
…e the defined ComponentDef rather than storing a ComponentId.

- Updated all tests
- Replaced others with Base.eval(Main, expr)
@rjplevin rjplevin requested a review from davidanthoff March 5, 2019 23:43
@codecov-io
Copy link

Codecov Report

Merging #415 into master will increase coverage by 0.33%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
+ Coverage   81.33%   81.67%   +0.33%     
==========================================
  Files          25       25              
  Lines        1897     1877      -20     
==========================================
- Hits         1543     1533      -10     
+ Misses        354      344      -10
Impacted Files Coverage Δ
src/core/model.jl 59.7% <100%> (+2.55%) ⬆️
src/core/types.jl 94.94% <100%> (ø) ⬆️
src/core/defs.jl 90% <100%> (+2.58%) ⬆️
src/core/defcomp.jl 87.5% <100%> (-0.24%) ⬇️
src/Mimi.jl 100% <100%> (ø) ⬆️
src/core/connections.jl 79.12% <100%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2eeca0f...84eb98f. Read the comment docs.

Copy link
Collaborator

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I in particular like that we can use getfield and don't have to go the eval route!

function __init__()
compdir = joinpath(dirname(@__FILE__), "components")
compdir = joinpath(@__DIR__, "components")
load_comps(compdir)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need to do this in `init``? With these fixes, shouldn't we just be able to include these files in the normal way now? We don't have to change this in this PR, just wondering...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I quickly tried to change this but MimiDice2013 complained about loading Mimi from cache. Rather than debug this now, I'll merge what we have.

@rjplevin rjplevin merged commit 71ba105 into master Mar 7, 2019
@rjplevin rjplevin deleted the eliminate_global_comp_dict branch March 7, 2019 22:14
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.

3 participants