feat: add blas/ext/base/ndarray/zxsa#12407
Conversation
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown_pkg_readmes
status: passed
- task: lint_markdown_docs
status: na
- task: lint_markdown
status: na
- task: lint_package_json
status: passed
- task: lint_repl_help
status: passed
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
Coverage Report
The above coverage report was generated for the changes in this PR. |
| var xbuf; | ||
| var x; | ||
|
|
||
| xbuf = new Complex128Array( [ -2.0, 1.0, 3.0, -5.0, 4.0, 0.0, -1.0, -3.0, 2.0, 1.0 ] ); |
There was a problem hiding this comment.
Aside: these tests are harder to review for the following reasons:
- You have moved everything to a single line, so visually grouping related elements takes more work.
- You have omitted the element comments, so the reviewer needs to mentally keep track of which elements are indexed.
- You have included non-sentinel values for those elements which are not indexed, which means that the reviewer needs to actually visually inspect the expected output, as it is not obvious which values are modified and which are not.
- You are forcing the reviewer to maintain a lot of mental state to determine whether tests are properly implemented, which leads the reviewer to put a lot of faith in hoping that test failures will have snuffed out mistakes and increases the likelihood of mistakes slipping through.
In general, you should strive to make the tests as obvious as possible. Otherwise, it just creates a drag on reviews and a burden on future readers.
There was a problem hiding this comment.
In short, there is a reason why we do things like
var x = [
1, // 0
1, // 0
0,
0,
2, // 1
2, // 1
0,
0,
3, // 2
3, // 2
...
];Yes, it creates more bookkeeping for the test author, and, yes, it increases likelihood of copy-paste mistakes, as happened in a couple of your recent PRs, but it also makes for much easier review and helps future readers quickly intuit expected behavior.
There was a problem hiding this comment.
cc @batpigandme this is something we should probably document.
There was a problem hiding this comment.
Sorry about that, @kgryte. I will definitely keep that in mind for future PRs and create another one for updating the existing packages. Apologies!
| t.strictEqual( actual, x, 'returns expected value' ); | ||
|
|
||
| expected = new Complex128Array( [ -7.0, 1.0, -2.0, -5.0, -1.0, 0.0, -6.0, -3.0, -3.0, 1.0 ] ); | ||
| t.deepEqual( xbuf, expected, 'returns expected value' ); |
There was a problem hiding this comment.
Actually, now that I look at these tests, this is wrong. t.deepEqual does not work the way you are expecting, as it doesn't know anything about accessor arrays.
You need to use isSameComplex128Array.
@anandkaranubc Mind fixing this across the various complex number packages you have added?
There was a problem hiding this comment.
Creating another one :)
The whitespace linter doc covered JSON fixture matrices but left the JavaScript test array case unaddressed. Strided test arrays present a distinct problem: flat single-line arrays force the reviewer to count strides, map storage positions to logical indices, and visually diff input against expected to find what changed. Document three moves anchored on kgryte review in PR stdlib-js#12407: break arrays one element per line, annotate indexed slots with trailing index comments (// 0, // 1, ...), and use sentinel zero values for non-indexed positions. Also add a matching heuristic bullet to the reviewer guidance section. Ref: stdlib-js#12407 (comment) --- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown_pkg_readmes status: skipped - task: lint_markdown_docs status: skipped - task: lint_markdown status: skipped - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: passed - task: lint_typescript_tests status: na - task: lint_license_headers status: passed ---
Resolves stdlib-js/metr-issue-tracker#677
Description
This pull request:
blas/ext/base/ndarray/zxsaRelated Issues
This pull request has the following related issues:
blas/ext/base/ndarray/zxsametr-issue-tracker#677Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
Used Cursor+VS Code code-generation tools to help assist with the feature.
@stdlib-js/reviewers