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

Remove getset #144

Merged
merged 7 commits into from
Dec 28, 2023
Merged

Remove getset #144

merged 7 commits into from
Dec 28, 2023

Conversation

serprex
Copy link
Contributor

@serprex serprex commented Dec 26, 2023

getset & its proc-macro-error dependencies seem unmaintained (jbaublitz/getset#84 (comment) jbaublitz/getset#87 (comment))

In particular, I'm not confident I can get upstream to update to syn 2

I also don't think getter/setter patterns make particular sense in Rust

Use #[non_exhausitive] in places to maintain capability to add fields without breaking semver

In some places I omitted #[non_exhaustive] where I didn't think it should be necessary, if you disagree let me know & I'll apply it more widely. Similarly, just let me know if you don't think #[non_exhaustive] is necessary

In some cases only getters were implemented, in those cases I implemented getter methods by hand & left the fields non-public

getset & its proc-macro-error dependencies seem unmaintained

In particular, I'm not confident I can get upstream to update to syn 2

I also don't think getter/setter patterns make particular sense in Rust

Use `_hidden: ()` in places to maintain capability to add fields without breaking semver
@serprex serprex force-pushed the remove-getset branch 2 times, most recently from 7efad49 to 9c7b942 Compare December 26, 2023 18:42
src/messages/copy.rs Outdated Show resolved Hide resolved
@sunng87
Copy link
Owner

sunng87 commented Dec 27, 2023

@serprex Thank you! I have checked all usage of #[non_exhausitive] I think it's just right for all necessary structs.

While I think it's ok to have direct member access over getter/setter, in particular when getset development become stalled, I need to think about impact of this because it's a breaking change.

@serprex
Copy link
Contributor Author

serprex commented Dec 27, 2023

If you don't want to have a breaking change I can look to create a minimal getset macro based on the rather straightforward use of the crate here

@sunng87
Copy link
Owner

sunng87 commented Dec 28, 2023

No worry. We can have breaking change during 0.x phase.

@sunng87 sunng87 merged commit 48a1b63 into sunng87:master Dec 28, 2023
6 checks passed
serprex added a commit to PeerDB-io/peerdb that referenced this pull request Jan 11, 2024
Changes in nexus due to pgwire 0.19 changes:
sunng87/pgwire#147
sunng87/pgwire#144
serprex added a commit to PeerDB-io/peerdb that referenced this pull request Jan 11, 2024
Changes in nexus due to pgwire 0.19 changes:
sunng87/pgwire#147
sunng87/pgwire#144
serprex added a commit to PeerDB-io/peerdb that referenced this pull request Jan 11, 2024
Changes in nexus due to pgwire 0.19 changes:
sunng87/pgwire#147
sunng87/pgwire#144
serprex added a commit to PeerDB-io/peerdb that referenced this pull request Jan 11, 2024
Changes in nexus due to pgwire 0.19 changes:
sunng87/pgwire#147
sunng87/pgwire#144
serprex added a commit to PeerDB-io/peerdb that referenced this pull request Jan 15, 2024
Changes necessary because of
1. sunng87/pgwire#144
2. sunng87/pgwire#147
serprex added a commit to PeerDB-io/peerdb that referenced this pull request Jan 15, 2024
Changes necessary because of
1. sunng87/pgwire#144
2. sunng87/pgwire#147

Tests were failing due to hanging in 0.19.0,
0.19.1 fixed hang: sunng87/pgwire#148
@sunng87
Copy link
Owner

sunng87 commented Feb 2, 2024

I just realized that a major drawback of pub struct + pub field access is it stops from adding new fields in semver non-breaking change. Because the initializer syntax Struct { field: value ... } is published as API.

@serprex
Copy link
Contributor Author

serprex commented Feb 2, 2024

That is prevented by

#[non_exhaustive]

@sunng87
Copy link
Owner

sunng87 commented Feb 2, 2024

Yes, we need to add this attribute carefully on each struct during 0.x phase. Adding this attribute is a breaking change too.

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.

2 participants