-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
Changing assert to become a class #58253
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
Changing assert to become a class #58253
Conversation
|
Review requested:
|
f728b4c to
132925f
Compare
|
I'm mildly concerned on the long term maintainability of this, as it essentially rewrites the module - backporting might lead to more churn than needed. You might get less churn by using the "old school" pattern: function Assert () {
}
Assert.prototype.notEqual = function () {}With this pattern, indentation would not change and this PR might be easier to review.
Can you clarify the need for this? Apart from that, this would need a CITGM check to verify it doesn't break end users in any way. |
|
Hey @miguelmarcondesf, thanks for the contribution 🚀 My 2 cents on this: while I understand the reasons behind this refactor, if I’m not mistaken, I don’t see any options actually being used in the class. So I’m wondering what the intended usage is for the new options we plan to introduce. The reason I’m asking is to provide a better context and justify any potential cons of rewriting the module (e.g., portability to other versions), while also providing an overview of what’s expected to be added. |
|
Hey @pmarchini, thanks for your thoughts!
I decided to open it in draft so I could receive more guidance on this, the PR already had a lot of modifications, and I wanted more perspectives so we could start this discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a glimpse at it so far, while it looks really good!
I definitely believe this is something we want to have!
The reason is that it's almost impossible to configure any algorithm behavior so far. Adding options to the current API is not really a way to go due to the overloading we have.
Using a class would finally allow to e.g., adjust how a diff should be visualized (all changes or cutting it off as it's done right now?), to adjust the algorithm checks in the deep equal comparison (e.g., should the prototype be checked, yes or no?). We definitely have many use cases for it being a class that users may adjust to produce the outcome of their needs that can't properly be addressed with our defaults.
I don’t see a clear benefit in terms of cognitive complexity or readability in the refactor itself.
Good point, @pmarchini!
I myself also try to only implement things when we make use of it.
A major concern has been the diff generated. I think we could add that as first option, since it's quite straight forward and it would address an issue.
132925f to
f1da641
Compare
Hey @mcollina thank you for the suggestion! I made the changes to the old-school pattern and if it makes sense, perhaps in future iterations we can migrate the functions to a version using a regular class. Also, for now first option added as suggested by @BridgeAR was the diff generated. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58253 +/- ##
==========================================
- Coverage 90.08% 90.03% -0.05%
==========================================
Files 641 648 +7
Lines 188718 191114 +2396
Branches 37022 37472 +450
==========================================
+ Hits 170000 172076 +2076
- Misses 11417 11661 +244
- Partials 7301 7377 +76
🚀 New features to boost your workflow:
|
1552a67 to
a5eb04c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already looking very promising! We need to document the class and the new AssertionError option, since both are exposed publicly.
We should also not expose the internal options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change is LGTM!
I am actually thinking it might be worth changing the default behavior for the non strict methods to be strict and add an option to change that back. That would prevent wrong usage which is quite likely right now when using the non-strict methods.
I do think changing that would be great before we land this.
@BridgeAR sounds good! I just pushed some changes related to that, thank you! |
3827903 to
41ae5be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! This is LGTM with my last comment being addressed!
I do think someone else should also have a look, since this is a bigger feature overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we modernize this and use ES6 classes?
41ae5be to
b664f8c
Compare
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
04b90b4 to
dc70085
Compare
@jasnell Thank you for pointing that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
Landed in 4f5d11e |
PR-URL: nodejs#58253 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #58253 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#58253 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#58253 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #58253 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

Changing assert to become a class
This PR refactors
assertfrom a method to a dedicated class. This change is motivated by the need for greater flexibility and configurability in assertion behavior.By turning
assertinto a class, we will be able to pass options that customize its behavior, such as doing specific checks, how the stack trace will look like, etc.Checklist
assertinto a class structure (old-school pattern).assertusages.cc @BridgeAR