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

added getter-methods to PdInfo #10

Merged
merged 1 commit into from
Aug 18, 2024

Conversation

CommanderStorm
Copy link
Contributor

@CommanderStorm CommanderStorm commented Aug 18, 2024

Currently, all fields of PdInfo are private.
The current way, to get access to them is to as_struct an instance and then compare the c-fields. This is somewhat cumbersome to do and not quite ergonomic.

=> I think making these fields pub is likely better

As an alternative, (if you'd like that more) I can make these fields into methods.
Another alternative would be to just publish parts of the fields, but that is something that I would want your oppinion on ^^

@sidcha
Copy link
Member

sidcha commented Aug 18, 2024

In the libosdp package, I try to hide libosdp_sys types and others such as CString in public API. To that end, I think it would be nicer to have accessor methods hiding these details for fields in this struct. What do you think?

Also, all public methods and members must have documentation.

@sidcha
Copy link
Member

sidcha commented Aug 18, 2024

Also, please git pull with --rebase to avoid necessary merge commits. It's not critical but kinda makes git history look clean.

@CommanderStorm CommanderStorm force-pushed the additional-pub branch 2 times, most recently from 6a83dbe to e8ef131 Compare August 18, 2024 20:10
@CommanderStorm
Copy link
Contributor Author

CommanderStorm commented Aug 18, 2024

Also, please git pull with --rebase to avoid necessary merge commits

Sorry, when I synced via githubs UI, that is the (non-changable) default for syncing via the Sync button ^^
Is fixed now

@CommanderStorm CommanderStorm changed the title made fields of PdInfo pub added getter-methods to PdInfo Aug 18, 2024
Copy link
Member

@sidcha sidcha left a comment

Choose a reason for hiding this comment

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

Thanks for this PR and others :)

@sidcha sidcha merged commit 246a910 into goToMain:master Aug 18, 2024
2 checks passed
@CommanderStorm CommanderStorm deleted the additional-pub branch August 18, 2024 21:15
@CommanderStorm
Copy link
Contributor Author

Thanks for this PR and others :)

Thanks a ton for reviewing them ^^

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