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

feat: add assert/has-same-constructor #1357

Closed
wants to merge 16 commits into from

Conversation

kaushiktak19
Copy link

@kaushiktak19 kaushiktak19 commented Feb 23, 2024

Resolves #815

Description

What is the purpose of this pull request?

This pull request:

  • Adds the package @stdlib/assert/has-same-constructor.

Related Issues

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

Copy link
Contributor

@stdlib-bot stdlib-bot left a comment

Choose a reason for hiding this comment

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

👋 Hi there! 👋

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

@Planeshifter Planeshifter self-requested a review February 23, 2024 20:28
@kgryte kgryte changed the title feat: adding @stdlib/assert/has-same-constructor #815 feat: adding assert/has-same-constructor Feb 24, 2024
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. labels Feb 24, 2024
var isArrayLikeObject = require( '@stdlib/assert/has-same-constructor' );
```

#### hasSameConstructor( value , value )
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#### hasSameConstructor( value , value )
#### hasSameConstructor( v1, v2 )


var DateObject1 = new Date();
var DateObject2 = new Date();
var StringObject = new String("Hello");
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using the String constructor, I suggest use more common examples. E.g., comparing a generic array [ 1, 2, 3 ], with a typed array instance (e.g., new Int32Array( [ 1, 2, 3 ] )). You can also use stdlib classes, such as @stdlib/complex/float64 and @stdlib/complex/float32.

// => false
```

</section>
Copy link
Member

Choose a reason for hiding this comment

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

You removed trailing sections for links, related packages, etc. Please append those here. We use them in various build processes, even if they are empty.

obj2 = new Date();
obj3 = new String( "Hello" );
obj4 = "World";
bool = hasSameConstructor( obj1, obj2 );
Copy link
Member

Choose a reason for hiding this comment

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

This is not what we want. Please see other benchmarks for how we measure things and read the benchmark harness README to get a better understanding of how benchmarks should be written: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/bench/harness.

Comment on lines 30 to 32
/**
* Benchmark the hasSameConstructor function when provided with same value types.
*/
Copy link
Member

Choose a reason for hiding this comment

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

No need for JSDoc comments here and below.


#### hasSameConstructor( v1, v2 )

Tests if two values have same constructor.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Tests if two values have same constructor.
Tests if two values have the same constructor.

// returns true

var obj1 = new Date();
var obj3 = new String("Hello");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var obj3 = new String("Hello");
var obj3 = new String( 'Hello' );

As elsewhere in this project, spaces and we use single quotes. Here and everywhere else.


</section>


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +65 to +66
<!-- eslint-disable no-new-wrappers -->

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- eslint-disable no-new-wrappers -->

var ComplexObj = new Complex64( 5.0, 3.0 );

var bool = hasSameConstructor( DateObj1, DateObj2 );
// => true
Copy link
Member

Choose a reason for hiding this comment

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

This is not how we do doctesting. See our doctesting guide: https://github.com/stdlib-js/stdlib/blob/develop/docs/doctest.md

Comment on lines +96 to +101

* * *

## See Also

- <span class="package-name">[`@stdlib/assert/has-iterator-symbol-support`][@stdlib/assert/has-iterator-symbol-support]</span><span class="delimiter">: </span><span class="description">detect native Symbol.iterator support.</span>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* * *
## See Also
- <span class="package-name">[`@stdlib/assert/has-iterator-symbol-support`][@stdlib/assert/has-iterator-symbol-support]</span><span class="delimiter">: </span><span class="description">detect native Symbol.iterator support.</span>

Comment on lines +113 to +118
<!-- <related-links> -->

[@stdlib/assert/has-iterator-symbol-support]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/assert/has-iterator-symbol-support

<!-- </related-links> -->

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- <related-links> -->
[@stdlib/assert/has-iterator-symbol-support]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/assert/has-iterator-symbol-support
<!-- </related-links> -->

var pkg = require( './../package.json' ).name;
var hasSameConstructor = require( './../lib' );

// MAIN //
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// MAIN //
// MAIN //

Comment on lines +4 to +5
If provided with two values with different constructors, the function returns `false`.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If provided with two values with different constructors, the function returns `false`.

@@ -0,0 +1,35 @@
{{alias}}( value )
Tests if two values have same constructor.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Tests if two values have same constructor.
Tests if two values have the same constructor.

Here and elsewhere.


Examples
--------
> var obj1 = new Date();
Copy link
Member

Choose a reason for hiding this comment

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

/**
* Test whether two provided values have the same constructor.
*
* @param {*} a - The first value to test.
Copy link
Member

Choose a reason for hiding this comment

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

We don't include type annotations in our TypeScript JSDoc comments. And also, this is not the format we use for parameter descriptions. Please refer to other packages for examples.

*
* @param {*} a - The first value to test.
* @param {*} b - The second value to test.
* @returns {boolean} Returns `true` if both values have the same constructor, else `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @returns {boolean} Returns `true` if both values have the same constructor, else `false`.
* @returns boolean indicating whether two values have the same constructor

* @param {*} a - The first value to test.
* @param {*} b - The second value to test.
* @returns {boolean} Returns `true` if both values have the same constructor, else `false`.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*

* var bool = hasSameConstructor(obj3, obj4);
* // returns true
*/

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

* // returns true
*/

declare function hasSameConstructor(a: any, b: any): boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
declare function hasSameConstructor(a: any, b: any): boolean;
declare function hasSameConstructor( a: any, b: any ): boolean;

* limitations under the License.
*/

import hasSameConstructor from './index';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import hasSameConstructor from './index';
import hasSameConstructor = require( './index' );

Please follow practices observed in other packages.


// The function returns a boolean...
{
hasSameConstructor(new Date(), new String("Hello")); // $ExpectType boolean
Copy link
Member

Choose a reason for hiding this comment

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

Spaces. Quotes.

'use strict';

var hasSameConstructor = require( './../lib' );
var Float64Array = require( '@stdlib/array/float64' );
Copy link
Member

Choose a reason for hiding this comment

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

Require statement order.

Comment on lines +26 to +28
* @param {*} a - The first value to test.
* @param {*} b - The second value to test.
* @returns {boolean} Returns `true` if both values have the same constructor, else `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Please follow conventions used in the project

* var bool = hasSameConstructor(obj3, obj4);
* // returns true
*/

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

* // returns true
*/

function hasSameConstructor(a, b) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function hasSameConstructor(a, b) {
function hasSameConstructor( a, b ) {

return a && b && a.constructor === b.constructor;
}

module.exports = hasSameConstructor;
Copy link
Member

Choose a reason for hiding this comment

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

Missing // EXPORTS // heading.

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@kaushiktak19 I suggest studying other packages in the project and trying to align your contribution as closely as possible to what is currently in the project.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Feb 24, 2024
@kgryte kgryte changed the title feat: adding assert/has-same-constructor feat: add assert/has-same-constructor Feb 24, 2024
@Planeshifter Planeshifter added the autoclose: Stale Pull request which should be auto-closed as considered stale. label Sep 30, 2024
@stdlib-bot
Copy link
Contributor

This pull request has been automatically closed because it has been inactive for an extended period after changes were requested. If you still wish to pursue this contribution, feel free to reopen the pull request or submit a new one.

We appreciate your interest in contributing to stdlib!

@stdlib-bot stdlib-bot closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoclose: Stale Pull request which should be auto-closed as considered stale. Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: Add @stdlib/assert/has-same-constructor
4 participants