docs: enhance jsdoc comments #511
Conversation
There was a problem hiding this comment.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
This reverts commit bfb2a2a.
This reverts commit e6976fc.
|
@shunkica Can you help me to understand why you are adding jsdoc for interfaces that are not implemented by xmldom? From my understanding just adding the jsdoc without implementing those interfaces only increases the bundle size of the package without providing any value fro the consumers. If you are planning to use them in some way, I would prefer, if possible, having small PRs that add usage and jsdoc one at a time. |
Ok. I will keep this PR strictly related to adding jsdoc for functionality which already exists in the lib. |
|
I commit this in my private repos. Will remove it once its ready.
…On Tue, Jul 25, 2023, 06:15 Christian Bewernitz ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .gitignore
<#511 (comment)>:
> @@ -11,3 +11,5 @@ coverage
# can be used to install node with the correct version when entering the directory
# https://github.com/direnv/direnv/wiki
8000
/Node
.envrc
+
+/.idea
Minor: ideally this belongs into your global gitignore file for your user
somewhere in your home directory, since it is related to your IDE/local
setup
—
Reply to this email directly, view it on GitHub
<#511 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACZM7OC2A2ECYTP22NO5XT3XR5B5RANCNFSM6AAAAAA2KTXYBI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@shunkica I'm planning to release 0.9.0 soon, since all things that were planned for it are ready. |
|
You should go ahead with the release. I am stretched too thin at the moment.
…On Tue, Aug 8, 2023, 16:46 Christian Bewernitz ***@***.***> wrote:
@shunkica <https://github.com/shunkica> I'm planning to release 0.9.0
soon, since all things that were planned for it are ready.
Can you provide a rough ETA for this PR?
Not that it's needed for the release, just to know whether it makes sense
to wait a bit longer.
—
Reply to this email directly, view it on GitHub
<#511 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACZM7OADU2ZQQ6H4FRB67HDXUJGLZANCNFSM6AAAAAA2KTXYBI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
What do you think about landing this PR now and you can file the next one once you have time for it? I mean as in "I fix this PR and land it". |
|
Did you review all the changes? Are you ok with this?
…On Tue, Aug 8, 2023, 18:32 Christian Bewernitz ***@***.***> wrote:
What do you think about landing this PR now and you can file the next one
once you have time for it?
—
Reply to this email directly, view it on GitHub
<#511 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACZM7OECGSWCWKCUTADG6SLXUJSZVANCNFSM6AAAAAA2KTXYBI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
well, currently this PR does to many things at once. I thought it would only add jsdoc, but that wouldn't explain failing tests. I would go ahead and revert all changes not related to jsdoc and would be able to land the PR this way. Would that work for you? One aspect I'm not sure about is whether it really makes sense to use the type alias And last but not least: I don't like the usage of TODOs should ideally be tracked as GitHub issues, so they can prioritized. Those are quite some changes, but I have them ready. |
# Conflicts: # lib/dom-parser.js # lib/sax.js
|
Sure, go ahead. I trust you will do what is right.
…On Tue, Aug 8, 2023, 19:56 Christian Bewernitz ***@***.***> wrote:
well, currently this PR does to many things at once.
I thought it would only add jsdoc, but that wouldn't explain failing tests.
I would go ahead and revert all changes not related to jsdoc and would be
able to land the PR this way. Would that work for you?
(Especially cleaning up all those note type and error code variables
should be at least one separate PR. changing the prototype chain of the
DOMException should also be an extra PR, and might be considered a breaking
one.)
One aspect I'm not sure about is wether it really makes sense to use the
type alias DOMString for string. Since the library doesn't have any extra
handling for the things mentioned in https://infra.spec.whatwg.org/#string
or https://webidl.spec.whatwg.org/#idl-DOMString I find it more confusing
then helpful to use a type alias.
So I would also have to undo the changes related to that, also because it
was not applied everywhere consistently.
And last but not least: I don't like the usage of @todo tags in doc
comments.
I started documenting the difference to the specified behavior as part of
the text in a reasoable way and prefer to keep that style.
TODOs should ideally be tracked as github issues, so they can prioritised.
(But of course it would be nice to not go ahead and create an issue for
every todo in the codebase. There might already be tickets for some of
them, in which case the TODO should be dropped from the code. And it's most
likely possible to group some of them into a lower amount of issues.)
Those are quite some changes, but I have them ready.
If it's OK for you I will push them to your branch, and land them.
—
Reply to this email directly, view it on GitHub
<#511 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACZM7OEIEMFJBKTTSWAJRILXUJ4VTANCNFSM6AAAAAA2KTXYBI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #511 +/- ##
=======================================
Coverage 92.77% 92.77%
=======================================
Files 7 7
Lines 2036 2036
Branches 530 530
=======================================
Hits 1889 1889
Misses 147 147
☔ View full report in Codecov by Sentry. |
|
Thank you for your efforts. |
This PR aims to add comprehensive JSDoc annotations to dom.js, based on XML DOM spec. The main goal of this enhancement is to improve readability, maintainability, and to provide a more in-depth understanding of the code.
The functionality of the library is unaffected.