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

Non exhaustive IniDefault #22

Merged
merged 2 commits into from
Oct 16, 2021
Merged

Conversation

vthib
Copy link
Contributor

@vthib vthib commented Oct 14, 2021

Mark the IniDefault struct as non exhaustive, as discussed here: #19 (comment)
The default values used in Ini::new and Ini::new_cs are moved inside IniDefault::default(), otherwise the workflow to create a new IniDefault and update a few values in it is quite cumbersome.

@QEDK QEDK added the enhancement New feature or request label Oct 15, 2021
@QEDK
Copy link
Owner

QEDK commented Oct 15, 2021

This is great! Removes a lot of code duplication which I've always wanted to come to.

@QEDK QEDK added this to the v3.0.0 milestone Oct 16, 2021
@QEDK
Copy link
Owner

QEDK commented Oct 16, 2021

@vthib can you rebase this on top of the current master, ty

Instead of having a derive(Default) on IniDefault, which allows creating
a struct with parameters that won't be ok, create an actual Default
impl for it. This also duplicates some code from new/new_cs.
This ensures that new fields can be added to it without it being
a breaking change.
@QEDK QEDK merged commit 311ea91 into QEDK:master Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants