Add Base.@kwdef to all system-related structs

[raphaelsaavedra]

This makes it easier to keep track of arguments when using these structs, as some of them have many arguments.

[codecov]

Codecov Report

Merging #19 (a79c549) into main (eb5b435) will increase coverage by 0.40%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
+ Coverage   79.59%   80.00%   +0.40%     
==========================================
  Files           2        2              
  Lines          98      105       +7     
==========================================
+ Hits           78       84       +6     
- Misses         20       21       +1     

Impacted Files	Coverage Δ
src/system.jl	53.48% <85.71%> (+6.26%)	⬆️

[nickrobinson251]

Closes #7

Needs tests :)

[raphaelsaavedra]

Needs tests :)

My plan was to create fake structs using the kwargs and test that they're equal to the ones not using kwargs, but that would require AutoHashEquals... is there another simple way of testing this?

[nickrobinson251]

we just need to test these methods exist

perhaps just @test Zone(; number=1, ...) isa Zone or z = Zone(; number=1, ...); @test z.number=1

[raphaelsaavedra]

The result was a bit ugly (circa 100 lines of code), but I covered all structs

[raphaelsaavedra]

Hmm, I think I'll need to do it a bit differently for Branch to avoid having to always define the transformer-only fields e.g. tap

[nickrobinson251]

should resistance and reactance have defaults too?

[raphaelsaavedra]

I don't think so, can't see any specific reason to have default values for these

[nickrobinson251]

i think the behaviour of the current constructors is that is_transformer is true if tap and angle are non-missing

perhaps that's the behaviour we should match here too?
perhaps remove is_transformer as a keyword and have something like

if ismissing(tap) && ismissing(angle) 
    is_transformer = false
elseif !ismissing(tap) && !ismissing(angle) 
    is_transformer = true
else
    ArgumentError("Transformers must have non-missing values for both `tap` and `angle`. Got `tap=$tap, angle=$angle`.")

?

Alternatively perhaps the constructors here are overly complex and we should check which are used in practice to see if we need them all ?

[nickrobinson251]

The result was a bit ugly (circa 100 lines of code), but I covered all structs

I suppose an alternative is to update the existing tests to use the new syntax e.g.

- zone1 = Zone(1, 1.0, 1.0, 1.0)
+ zone1 = Zone(number=1, regulation=1.0, etc1=1.0, etc2=1.0)

[raphaelsaavedra]

Does this look good to you now @nickrobinson251?

[nickrobinson251]

what do you think of update the existing tests to use the new syntax rather than adding a bunch of more tests?

I also wonder if we should open an issue about reviewing which Brnach constructors are necessary - what do you think?

Otherwise LGTM

[raphaelsaavedra]

what do you think of update the existing tests to use the new syntax rather than adding a bunch of more tests?

I was thinking that the current way covers both cases – but in the end I think we get the same coverage if we update the existing ones? I can do that

[nickrobinson251]

thanks - LGTM!

[raphaelsaavedra]

Had a test failing due to resistance, reactance having default values, which I'm not sure should be the case. I'll leave this unchanged in this PR; opened #21 to track it