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

Merged
merged 25 commits into from
Aug 20, 2024
Merged

C++ improvements #63

merged 25 commits into from
Aug 20, 2024

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 force-pushed the markkoh/cpp-improvements branch 4 times, most recently from 6ead919 to 406b4e0 Compare March 29, 2024 22:35
@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.

CONTRIBUTING.md Outdated
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

@markkohdev
Copy link
Contributor Author

This should help us get closer to solving #11 and #3 (although we may address this first in the Java bindings)

@@ -37,10 +37,9 @@ jobs:
steps:
- uses: actions/checkout@v3
- name: Check C++ Formatting
uses: jidicula/clang-format-action@v4.4.1
uses: jidicula/clang-format-action@v4.11.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add c++ action container

@markkohdev markkohdev force-pushed the markkoh/cpp-improvements branch 13 times, most recently from a875599 to bba8a5a Compare August 19, 2024 06:05
@markkohdev markkohdev force-pushed the markkoh/cpp-improvements branch 2 times, most recently from 97e216f to 6cc7614 Compare August 19, 2024 21:54
@markkohdev markkohdev force-pushed the markkoh/cpp-improvements branch 7 times, most recently from 0ce71d7 to 9c6af20 Compare August 19, 2024 22:25
@markkohdev markkohdev merged commit a4902b8 into main Aug 20, 2024
57 checks passed
@markkohdev markkohdev deleted the markkoh/cpp-improvements branch August 20, 2024 17:27
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.

3 participants