Skip to content

feat: add assert/is-same-date-object #1348

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 23 commits into from
Mar 3, 2024
Merged

feat: add assert/is-same-date-object #1348

merged 23 commits into from
Mar 3, 2024

Conversation

adityacodes30
Copy link
Contributor

@adityacodes30 adityacodes30 commented Feb 22, 2024 •
edited
Loading

Resolves #1337

Description

What is the purpose of this pull request?

This PR adds @stdlib/assert/is-same-date-object

Related Issues

Does this pull request have any related issues?

Yes it solves #1337

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.

This implementation uses the getTime() function to compare upto the same timestamp that covers different ways of initialising the Date object

Checklist

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

@stdlib-js/reviewers

stdlib-bot
stdlib-bot reviewed Feb 22, 2024
View reviewed changes
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. 🏃 💨

@kgryte kgryte changed the title feat: add @stdlib/assert/is-same-date-object - #1337 feat: add assert/is-same-date-object Feb 22, 2024
@kgryte
Copy link
Member

kgryte commented Feb 22, 2024

@adityacodes30 You mind ticking the box indicating that you've read the contributing guidelines? Thanks!

@adityacodes30
Copy link
Contributor Author

@adityacodes30 You mind ticking the box indicating that you've read the contributing guidelines? Thanks!

done

@kgryte
Copy link
Member

kgryte commented Feb 22, 2024

Thanks for submitting this PR. We'll try to review within the next day or so.

kgryte
kgryte reviewed Feb 22, 2024
View reviewed changes
lib/node_modules/@stdlib/assert/is-same-date-object/README.md Outdated
Tests if two arguments are both valid date objects and both resolve to the same timestamp.

```javascript
var d1 = new Date(2024, 11, 31, 23, 59, 59, 999);
Copy link
Member

Choose a reason for hiding this comment

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

@adityacodes30 Would you mind updating here and elsewhere to adhere to common project conventions? Namely, with some exception, we prefer spaces within brackets. E.g.,

Suggested change
var d1 = new Date(2024, 11, 31, 23, 59, 59, 999);
var d1 = new Date( 2024, 11, 31, 23, 59, 59, 999 );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do that pronto

kgryte
kgryte reviewed Feb 22, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 22, 2024
View reviewed changes
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. Needs Review A pull request which needs code review. Needs Changes Pull request which needs changes before being merged. labels Feb 23, 2024
@adityacodes30
Copy link
Contributor Author

@kgryte ive made the changes , let me know if anything else needs to be done

kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
lib/node_modules/@stdlib/assert/is-same-date-object/benchmark/benchmark.length.js Outdated
var i;
b.tic();
for ( i = 0; i < b.iterations; i++ ) {
date2.setMilliseconds( i ); // Change the date slightly each iteration
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the intent to ensure we compare different dates for each iteration in order to avoid certain compiler optimizations. I think we can do better by simply using a pre-allocated list of values containing a list of date objects and then in the loop, just use cheap modulo arithmetic to pull out a new date2 for comparison. If you hunt enough, you should be able to find a similar approach elsewhere in the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got that, i'll make the changes and pull the 'date2' from a values array.

that aside i'm curious about your usage of the word 'cheap' , is there a high cost operation here ? if yes then which one and how. Would highly appreciate your knowledge

Copy link
Member

Choose a reason for hiding this comment

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

A method call which mutates an object is likely to be slower than an operator (e.g., %) which is implemented in hardware. Here, we want to avoid doing operations which have perf costs similar to what we are attempting to test. Otherwise, perf results have conflating factors and it becomes harder to separate signal from noise when comparing across implementations and/or packages. Ideally, any boilerplate code around the operation of focus should be at least an order of magnitude faster, preferably more, such that any additional costs are negligible.

kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 29, 2024
View reviewed changes
adityacodes30 and others added 7 commits March 1, 2024 14:21
@adityacodes30 @kgryte
Update lib/node_modules/@stdlib/assert/is-same-date-object/docs/repl.txt
8dfcc54 
Co-authored-by: Athan <[email protected]>
Signed-off-by: Aditya Sapra <[email protected]>
@adityacodes30
feat(@stdlib/assert): add isDateObject function,requested changes,std…
8c73111 
…lib-js#1337
@adityacodes30
feat(@stdlib/assert): add isDateObject function,requested changes,std…
f1710dd 
…lib-js#1337
@adityacodes30 @kgryte
Update lib/node_modules/@stdlib/assert/is-same-date-object/benchmark/…
bc9877a 
…benchmark.js

Co-authored-by: Athan <[email protected]>
Signed-off-by: Aditya Sapra <[email protected]>
@adityacodes30 @kgryte
Update lib/node_modules/@stdlib/assert/is-same-date-object/docs/repl.txt
59fb3c9 
Co-authored-by: Athan <[email protected]>
Signed-off-by: Aditya Sapra <[email protected]>
@adityacodes30
feat(@stdlib/assert): add isDateObject function,requested changes,std…
bb1a8ae 
…lib-js#1337
@adityacodes30
Merge branch 'is-same-date-object' of https://github.com/adityacodes3…
87f1275 
…0/stdlib into is-same-date-object
@adityacodes30
Copy link
Contributor Author

@kgryte ive made the changes , i see a lot of co-sign commits, let me know if you want me to squash them

Planeshifter
Planeshifter approved these changes Mar 3, 2024
View reviewed changes
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.

Let's get this in, thanks!

Suggested a few remaining changes, but will merge once CI has passed.

@Planeshifter
Update lib/node_modules/@stdlib/assert/is-same-date-object/package.json
34d7024 
Signed-off-by: Philipp Burckhardt <[email protected]>
@Planeshifter
Update lib/node_modules/@stdlib/assert/is-same-date-object/docs/repl.txt
7445142 
Signed-off-by: Philipp Burckhardt <[email protected]>
@Planeshifter
Update lib/node_modules/@stdlib/assert/is-same-date-object/docs/repl.txt
0cefc0e 
Signed-off-by: Philipp Burckhardt <[email protected]>
@Planeshifter
Update lib/node_modules/@stdlib/assert/is-same-date-object/examples/i…
063b38e 
…ndex.js

Signed-off-by: Philipp Burckhardt <[email protected]>
@Planeshifter
Update lib/node_modules/@stdlib/assert/is-same-date-object/examples/i…
fdf2ab4 
…ndex.js

Signed-off-by: Philipp Burckhardt <[email protected]>
@Planeshifter
Update lib/node_modules/@stdlib/assert/is-same-date-object/examples/i…
b0cda06 
…ndex.js

Signed-off-by: Philipp Burckhardt <[email protected]>
@Planeshifter
Update lib/node_modules/@stdlib/assert/is-same-date-object/examples/i…
50a5189 
…ndex.js

Signed-off-by: Philipp Burckhardt <[email protected]>
@Planeshifter
Update lib/node_modules/@stdlib/assert/is-same-date-object/examples/i…
9aadb60 
…ndex.js

Signed-off-by: Philipp Burckhardt <[email protected]>
@Planeshifter
Update lib/node_modules/@stdlib/assert/is-same-date-object/package.json
d38aa8b 
Signed-off-by: Philipp Burckhardt <[email protected]>
@adityacodes30
Copy link
Contributor Author

seems like there are some lint errors , let me take a look at em

@Planeshifter
Copy link
Member

@adityacodes30 Was just a duplicate var declaration; now everything passes. Merging...

@Planeshifter Planeshifter merged commit 2bde735 into stdlib-js:develop Mar 3, 2024
bad-in-coding pushed a commit to bad-in-coding/stdlib that referenced this pull request Mar 7, 2024
@adityacodes30 @kgryte @Planeshifter
feat: add assert/is-same-date-object
9ad9c12 
PR-URL: stdlib-js#1348 
Closes: stdlib-js#1337

---------

Signed-off-by: Aditya Sapra <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Co-authored-by: Athan Reines <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
@adityacodes30 adityacodes30 mentioned this pull request Mar 25, 2024
Closed
6 tasks
@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
Reviewers

@Planeshifter Planeshifter Planeshifter approved these changes

@kgryte kgryte kgryte requested changes

@stdlib-bot stdlib-bot stdlib-bot left review comments

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC]: add @stdlib/assert/is-same-date-object
4 participants
@adityacodes30 @kgryte @Planeshifter @stdlib-bot