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

Derive PartialEq for Update structs #118

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

longsleep
Copy link
Collaborator

To allow easy checks for "empty" updates, this change adds the PartialEq derive to the generated Update structs. This allows comparison with Default::default() to check if an update is empty.

Fixes: #117

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

you will need to run ./test/test_all.sh and commit the changes if they look correct

@longsleep longsleep force-pushed the longsleep-derive-partialeq-update branch 2 times, most recently from d5df07f to 97a571a Compare November 7, 2023 18:18
@longsleep longsleep requested a review from hasezoey November 7, 2023 18:19
@longsleep
Copy link
Collaborator Author

you will need to run ./test/test_all.sh and commit the changes if they look correct

Sorry forgot about that - done now and also rebased.

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

change looks good to me, only really missing a entry in the CHANGELOG (forgot to include this in my message before)

also note that i currently hold off merging PRs to allow Wulf to rebase & merge his relatively big PR, unless otherwise indicated (#103 (comment))

@longsleep
Copy link
Collaborator Author

change looks good to me, only really missing a entry in the CHANGELOG (forgot to include this in my message before)

OK this is a bit strange to require changes of the CHANGELOG with the merge request. That will conflict pretty much every time then. Normally the CHANGELOG is updated in regular intervals for releases and I do not see a point in having "unreleased" changes in the CHANGELOG (after all there is git to see those). Just my two cents on this - I will happily update the changelog in a dedicated commit if that makes things easier for you. Just let me know.

@hasezoey
Copy link
Collaborator

hasezoey commented Nov 8, 2023

OK this is a bit strange to require changes of the CHANGELOG with the merge request. That will conflict pretty much every time then.

this is normally true, but we (at least currently) dont have automated releases and later guessing on what something changed is not the best option, including forgetting to entry a change at all.

also currently every PR basically conflicts with every other PR because of the generated tests already.

To allow easy checks for "empty" updates, this change adds the PartialEq
derive to the generated Update structs. This allows comparison with
`Default::default()` to check if an update is empty.

Fixes: Wulf#117
@longsleep longsleep force-pushed the longsleep-derive-partialeq-update branch from 97a571a to 43ea01b Compare November 10, 2023 11:09
@hasezoey hasezoey merged commit c4697b2 into Wulf:main Nov 10, 2023
This was referenced Nov 10, 2023
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.

Add PartialEq derive to generated "Update" structs
2 participants