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.

Sign up for GitHub

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

Jump to bottom

Split C header #290

Closed
wants to merge 1 commit into from
Closed

Split C header #290

wants to merge 1 commit into from

Conversation

markaren
Copy link
Contributor

@markaren markaren commented Jul 1, 2019

Closes #289

eidekrist
eidekrist reviewed Jul 1, 2019
View reviewed changes
Copy link
Member

@eidekrist eidekrist left a comment

Choose a reason for hiding this comment

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

I'm quite okay with this, but my feelings on this topic aren't particularly strong. I would however like to see a companion PR for cse-server-go as it will be affected by these changes.

@markaren
Copy link
Contributor Author

markaren commented Jul 1, 2019

I'm in no rush with this one. But I feel it's needed as the cse.h continues to grow in size.

As for the cse-server-go companion PR. I hope someone but me are up for the task.

@kyllingstad
Copy link
Member

I agree that cse.h is getting unwieldly and that it might be time to split it up. However, I disagree somewhat with the concrete split that is suggested here. I think header files should be organised by topic, not by symbol type. So for example, you could have one header for slave-related stuff, one for observer-related stuff, one for error handling, etc. – basically, how the C++ library is organised.

@markaren
Copy link
Contributor Author

Closing PR, but not the issue.

@markaren markaren closed this Aug 20, 2019
@markaren markaren deleted the feature/289-split-C-header branch August 21, 2019 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eidekrist eidekrist eidekrist left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split C library header
3 participants
@markaren @kyllingstad @eidekrist