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

Use keywords for creating parameter structs #1566

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Use keywords for creating parameter structs #1566

merged 1 commit into from
Jun 20, 2024

Conversation

visr
Copy link
Member

@visr visr commented Jun 19, 2024

By putting the kwdef macro in front of our struct definitions we can create them with keywords and set defaults if applicable. This helps with readability and makes it less likely to pass the wrong argument due to a wrong order. E.g. compare this test code before and after:

basin = Ribasim.Basin(
    NodeID.(:Basin, [1], 1),
    [NodeID[]],
    [NodeID[]],
    zeros(1),
    zeros(1),
    zeros(1),
    zeros(1),
    zeros(1),
    zeros(1),
    zeros(1),
    [storage_to_level],
    [level_to_area],
    demand,
    StructVector{Ribasim.BasinTimeV1}(undef, 0),
)
basin = Ribasim.Basin(;
    node_id = NodeID.(:Basin, [1], 1),
    storage_to_level = [storage_to_level],
    level_to_area = [level_to_area],
    demand,
    time = StructVector{Ribasim.BasinTimeV1}(undef, 0),
)```

@Jingru923 Jingru923 self-requested a review June 20, 2024 08:42
@Jingru923
Copy link
Contributor

Looks good to me. In terms of which struct to make use of this macro and to have default parameter, I trust you. For the unit tests, I don't see any unit tests that can make use of this being left out. Approve
(I also got your failing test on TC passed)

@Jingru923 Jingru923 merged commit 9c8898c into main Jun 20, 2024
25 checks passed
@Jingru923 Jingru923 deleted the kwdef branch June 20, 2024 12:35
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.

None yet

2 participants