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

Refactoring #410

Merged
merged 1 commit into from
Sep 18, 2022
Merged

Refactoring #410

merged 1 commit into from
Sep 18, 2022

Conversation

dyashuni
Copy link
Contributor

@dyashuni dyashuni commented Sep 4, 2022

Added a new method to replace the deleted elements and tests
Refactoring:

  • deleted unused function searchKnnInternal
  • improved code readability

Edit:
I left only refactoring

@yurymalkov
Copy link
Member

Can you split it into 2 PRs?
It is hard to review both refactoring code changes (which don't require attention) and meaningful changes (which require a lot of attention).

@dyashuni dyashuni force-pushed the replacement branch 2 times, most recently from c810d69 to dab9e99 Compare September 6, 2022 10:26
@dyashuni dyashuni changed the title Replacement of deleted elements Refactoring Sep 7, 2022
Copy link
Member

@yurymalkov yurymalkov left a comment

Choose a reason for hiding this comment

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

The PR has some simple refactoring (I guess automated or semi-automated), fixing some issues with tests and removal of some code duplicates.
The automated refactoring together is ugly diff makes it hard to spot the meaningful changes (and it does not make much sense to check the automated refactored code).
Maybe, can you provide a list of those changes, comment them or split the PR even more?

python_bindings/bindings.cpp Outdated Show resolved Hide resolved
python_bindings/bindings.cpp Outdated Show resolved Hide resolved
@dyashuni
Copy link
Contributor Author

dyashuni commented Sep 16, 2022

I reverted changes in python tests and code duplication changes. Now only refactoring is here.

  • code formatting is based on the cpplint that follows Google C++ style guide. I used a VS Code plugin. The main changes: adding and removing redundant spaces, new lines, remove indents within namespaces, brackets "{}" positions in if/else statements
  • changing declaration order: data members first
  • remove unused function searchKnnInternal

@yurymalkov
Copy link
Member

Thank you! With a quick look seems to be good. We would need to run the performance tests before the release though.

Please merge pull request with the "squash and merge" option, so it would be a single commit.

@dyashuni dyashuni force-pushed the replacement branch 2 times, most recently from d871f58 to 10e75a2 Compare September 18, 2022 08:04
@dyashuni
Copy link
Contributor Author

Squashed changes in one commit. Performance tests do not show any regress.

@dyashuni dyashuni merged commit 6d28ec0 into nmslib:develop Sep 18, 2022
@dyashuni dyashuni deleted the replacement branch September 18, 2022 09:57
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