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

refactor: update blas/ext/base/dapxsumkbn2 to follow current project conventions #1978

Merged
merged 11 commits into from
Mar 26, 2024
Merged

refactor: update blas/ext/base/dapxsumkbn2 to follow current project conventions #1978

merged 11 commits into from
Mar 26, 2024

Conversation

kumarsuraj212003
Copy link
Contributor

Resolves #1467 .

Description

What is the purpose of this pull request?

refactor blas/ext/base/dapxsumkbn2 to follow current project conventions

This pull request:

Related Issues

#788, #1152

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@stdlib-bot stdlib-bot added the BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). label Mar 21, 2024
Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
@Planeshifter Planeshifter added Needs Review A pull request which needs code review. Native Addons Issue involves or relates to Node.js native add-ons. C Issue involves or relates to C. labels Mar 23, 2024
@Planeshifter Planeshifter self-requested a review March 23, 2024 22:05
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Left a few comments; mainly the manifest.json file has to be updated. Please have a look at how we have been updating it in some of the linked PRs in the RFC.

Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
@kumarsuraj212003
Copy link
Contributor Author

kumarsuraj212003 commented Mar 24, 2024

@Planeshifter thanks for review, I added suggested changes, can you please tell me why my run_affected_tests / Run affected tests (pull_request) test is failling or Do I need to update any particular dependencies?

@kgryte
Copy link
Member

kgryte commented Mar 24, 2024

@kumarsuraj212003 The manifest.json file still needs updating.

Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
@kumarsuraj212003
Copy link
Contributor Author

@kgryte run_affected_tests / Run affected tests (pull_request) is still failing. Could you please investigate the error.It helps me alot. Thank you

@kgryte
Copy link
Member

kgryte commented Mar 24, 2024

Done. You're missing an include and the manifest.json file is missing a dep.

Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
@kumarsuraj212003
Copy link
Contributor Author

@kgryte I think I understood what you wanted me to correct, and I have made the necessary adjustments, including adding the missing include and updating the dependencies in the manifest.json file. Thank you again for helping me identify and resolve my errors.

Signed-off-by: Suraj kumar <125961509+kumarsuraj212003@users.noreply.github.com>
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Thanks for getting this PR into a good shape! LGTM. I will merge it shortly.

@Planeshifter Planeshifter merged commit 7ba3296 into stdlib-js:develop Mar 26, 2024
8 checks passed
@Planeshifter Planeshifter removed the Needs Review A pull request which needs code review. label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). C Issue involves or relates to C. Native Addons Issue involves or relates to Node.js native add-ons.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: refactor blas/ext/base/dapxsumkbn2 to follow current project conventions
4 participants