Remove first and last keyword arguments

[corakingdon]

#567

[rjplevin]

I think one if/then regarding before/after should not have been deleted. See comment...

[codecov-io]

Codecov Report

Merging #578 into master will decrease coverage by 0.03%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #578      +/-   ##
==========================================
- Coverage   77.46%   77.43%   -0.04%     
==========================================
  Files          39       39              
  Lines        2574     2566       -8     
==========================================
- Hits         1994     1987       -7     
+ Misses        580      579       -1

Impacted Files	Coverage Δ
src/core/model.jl	81.81% <ø> (ø)	⬆️
src/core/instances.jl	78.3% <0%> (ø)	⬆️
src/core/defs.jl	78.66% <66.66%> (-0.54%)	⬇️
src/core/defcomp.jl	89.72% <0%> (+0.68%)	⬆️

---

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 adcb0bf...cc1c23a. Read the comment docs.

[corakingdon]

@rjplevin I removed the use of first and last keywords in the various add_comp! and replace! methods. Do you think I should also remove the first and last fields from the ComponentDef, ComponentInstance, and ModelComponentDef classes for this pull request? example:

@class mutable ComponentDef <: NamedObj begin
    comp_id::Union{Nothing, ComponentId}    # allow anonynous top-level (composite) 
    ComponentDefs (must be referenced by a ModelDef)
    comp_path::Union{Nothing, ComponentPath}
    dim_dict::OrderedDict{Symbol, Union{Nothing, Dimension}}
    namespace::OrderedDict{Symbol, Any}
    first::Union{Nothing, Int}
    last::Union{Nothing, Int}
    is_uniform::Bool

    ...

end

[rjplevin]

Agreed, no need to leave artifacts behind.

[corakingdon]

@rjplevin It turns out that deleting the first and last fields from those three classes is a bit more involved, since there are also these first_period and last_period methods for each type:

first_period(obj::AbstractComponentDef) = obj.first
first_period(obj::AbstractComponentInstance) = obj.first
first_period(root::AbstractCompositeComponentDef, comp::AbstractComponentDef) = @or(first_period(comp), first_period(root))

These methods get used all throughout the code when setting parameters, making connections, and allocating timestep arrays (even though for now the first period is always just the first year of the time index, since the first/last functionality was disabled). I would have to delete and work around all the calls to first_period in these places, but I'm wondering if it would be better to leave this untouched for v1.0.0, and then deal with them later when we might re-implement the first/last capabilities in a different way, as we were discussing at the end of this issue: #422

Thoughts?

[rjplevin]

I’m out running errands today, so slow in responding. I think leaving it is reasonable for now. Cheers, Rich

…

On Oct 16, 2019, at 12:06 PM, Cora Kingdon ***@***.***> wrote: @rjplevin It turns out that deleting the first and last fields from those three classes is a bit more involved, since there are also these first_period and last_period methods for each type: first_period(obj::AbstractComponentDef) = obj.first first_period(obj::AbstractComponentInstance) = obj.first first_period(root::AbstractCompositeComponentDef, comp::AbstractComponentDef) = @or(first_period(comp), first_period(root)) These methods get used all throughout the code when setting parameters, making connections, and allocating timestep arrays (even though for now the first period is always just the first year of the time index, since the first/last functionality was disabled). I would have to delete and work around all the calls to first_period in these places, but I'm wondering if it would be better to leave this untouched for v1.0.0, and then deal with them later when we might re-implement the first/last capabilities in a different way, as we were discussing at the end of this issue: #422 Thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.