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 vector module for better maintainability and extensibility. #371

Merged
merged 1 commit into from
Feb 18, 2023

Conversation

metasim
Copy link
Contributor

@metasim metasim commented Feb 11, 2023

  • I agree to follow the project's code of conduct.
  • [NA] I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Refactoring to improve maintainability of vector module.

Changes include:

  • Grouping methods of similar purpose into separate files
  • Putting associated tests in the same file as the implementation
  • Adding or expanding documentation on existing methods
  • Normalizing documentation patterns and format.

Intent was to not have any breaking changes.

Copy link
Member

@lnicola lnicola 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 I can't say for sure 😅.

One naming nit: should we use something like set instead of binary? And formats (:bike: :house:) instead of text?

@metasim
Copy link
Contributor Author

metasim commented Feb 13, 2023

One naming nit

Not a nit... I've already renamed those 5 times. 🙈

Changes include:
* Grouping methods of similar purpose into separate files
* Putting associated tests in the same file as the implementation
* Adding or expanding documentation on existing methods
* Normalizing documentation patterns and format.
@metasim metasim marked this pull request as ready for review February 16, 2023 14:14
@metasim
Copy link
Contributor Author

metasim commented Feb 16, 2023

@lnicola @jdroenner I'm going to merge this, and then in a separate PR implement union, difference, symmetric_difference, etc. in the new set module.

@lnicola
Copy link
Member

lnicola commented Feb 18, 2023

[NA] I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

image

Anyway, we can probably merge this.

bors d+

@bors
Copy link
Contributor

bors bot commented Feb 18, 2023

✌️ metasim can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@lnicola
Copy link
Member

lnicola commented Feb 18, 2023

Actually, maybe there's no breaking changes?

bors r+

@bors bors bot merged commit 40bbffd into georust:master Feb 18, 2023
@metasim metasim deleted the feature/vector-refactor branch February 18, 2023 18:12
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