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

C++ improvements #63

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

C++ improvements #63

wants to merge 13 commits into from

Conversation

markkohdev
Copy link
Contributor

@markkohdev markkohdev commented Mar 22, 2024

Description

In this PR we will:

  • Clean up the C++ module structure
  • Add CMake for standalone C++ compilation
  • Improve formatting within the c++ module
  • Add doctest testing framework for C++ unit tests
  • Update documentation to be more comprehensive around building, testing, etc.
  • Add a github pull request template :)

Changes Made

C++

Main changes:

  • Break cpp dir into src and test directories
  • Add cmake and documentation around it
  • Add an initial test
  • Run clang-format with new 120 line length standard

Python

  • Remove c++ formatting of python bindings from tox
  • Remove voyager-headers link since it doesn't seem to be needed
  • Update location of C++ files

Java

  • Update location of C++ files
  • Run formatter
  • Update docs

Testing

Checklist

  • My code follows the code style of this project.
  • I have added and/or updated appropriate documentation (if applicable).
  • All new and existing tests pass locally with these changes.
  • I have run static code analysis (if available) and resolved any issues.
  • I have considered backward compatibility (if applicable).
  • I have confirmed that this PR does not introduce any security vulnerabilities.

Additional Comments

@markkohdev markkohdev changed the title [WIP] C++ improvements C++ improvements Apr 1, 2024
@markkohdev markkohdev marked this pull request as ready for review April 1, 2024 18:19
@@ -0,0 +1 @@
cpp/.clang-format
Copy link
Contributor Author

Choose a reason for hiding this comment

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

softlink to allow the clang-format github action to work correctly

Copy link
Member

@psobot psobot left a comment

Choose a reason for hiding this comment

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

LGTM overall, but if we're adding C++ tests (even just the one) in this PR, IMO we should add them to the GitHub Actions workflow so they run on CI before merging this.

mvn package
```

this will update the java documentation located in [docs/java/](https://github.com/spotify/voyager/tree/main/docs/java).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this will update the java documentation located in [docs/java/](https://github.com/spotify/voyager/tree/main/docs/java).
This will update the java documentation located in [docs/java/](https://github.com/spotify/voyager/tree/main/docs/java).

@alexander-riss
Copy link

without any pressure - is there a timeframe for merging this? just asking for our own planning on when to start integrating these changes

@alexander-riss
Copy link

just a friendly ping - any update on this? do you think it makes sense for potential users to start picking up your branch directly instead of waiting for this to be merged? @markkohdev

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.

None yet

3 participants