Skip to content

Aligning the code with contributing guidelines #42

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
Merged
merged 18 commits into from
Feb 17, 2025
Merged

Aligning the code with contributing guidelines #42

merged 18 commits into from
Feb 17, 2025

Conversation

shakedregev
Copy link
Collaborator

This is my first pass at the cleanup of the code. It's not ready yet.

  1. There's a stylistic choice of inline functions that does not really go with rest of the code. I want to understand the rationale or change it to regular functions.
  2. Most functions do not have explanations of their parameters (and are not marked in or out)

@shakedregev shakedregev requested a review from reid-g January 16, 2025 14:59
@shakedregev shakedregev self-assigned this Jan 16, 2025
@shakedregev shakedregev marked this pull request as draft January 16, 2025 14:59
@shakedregev shakedregev requested a review from pelesh January 16, 2025 15:25
@shakedregev
Copy link
Collaborator Author

I further noticed that COO_Matrix is incorrectly defined with an underscore. Is this on purpose?
I put comments in the code, you can search for "SR".

@pelesh
Copy link
Collaborator

pelesh commented Jan 18, 2025

@shakedregev, please fix build error (see CI pipeline).

@shakedregev
Copy link
Collaborator Author

@shakedregev, please fix build error (see CI pipeline).

Fixed

@pelesh pelesh force-pushed the shaked/code_cleanup branch from 3e9da94 to b337023 Compare January 22, 2025 17:55
pelesh
pelesh reviewed Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

Left a few nitpicking comments. Generally looks good. Mark when it is ready to merge.

pelesh
pelesh previously requested changes Jan 28, 2025
View reviewed changes
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

  • Document COO_Matrix constructors.
  • Document method variables
  • Break lines that are too long.

@pelesh pelesh added documentation Improvements or additions to documentation enhancement New feature or request labels Feb 11, 2025
@shakedregev shakedregev marked this pull request as ready for review February 11, 2025 16:15
@shakedregev shakedregev requested a review from pelesh February 11, 2025 16:15
@shakedregev shakedregev changed the title Draft: Aligning the code with contributing guidelines Aligning the code with contributing guidelines Feb 11, 2025
@alexander-novo
Copy link
Collaborator

I'm not sure this is the right place to discuss this, but since there is a lot of attention being put on COO_Matrix, I wanted to mention that I noticed that there isn't actually an include guard on the header file.

@shakedregev shakedregev requested a review from pelesh February 14, 2025 20:14
@shakedregev shakedregev dismissed pelesh’s stale review February 17, 2025 15:12

changes addressed

pelesh
pelesh requested changes Feb 17, 2025
View reviewed changes
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

This is ready to merge. Great job! Only two minor changes required.

reid-g
reid-g approved these changes Feb 17, 2025
View reviewed changes
Copy link
Collaborator

@reid-g reid-g left a comment

Choose a reason for hiding this comment

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

Handle the previous comments by pelesh then it looks good.

@pelesh pelesh self-requested a review February 17, 2025 22:52
@pelesh pelesh merged commit bc3f969 into develop Feb 17, 2025
@pelesh pelesh deleted the shaked/code_cleanup branch March 14, 2025 03:36
pelesh added a commit that referenced this pull request Apr 14, 2025
@shakedregev @pelesh
Aligning the code with contributing guidelines (#42)
d616929 
* fixed some style issues and variable name selection

* fixed guard for hpp file

* fixed typos


---------

Co-authored-by: pelesh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pelesh pelesh pelesh approved these changes

@reid-g reid-g reid-g approved these changes

Assignees

@shakedregev shakedregev

Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shakedregev @pelesh @alexander-novo @reid-g