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

Add comment-documentation for several key Theia utility classes #13324

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

jcortell68
Copy link
Contributor

@jcortell68 jcortell68 commented Jan 26, 2024

What it does

Adds comment-documentation for several key Theia utility classes

  • DisposableCollection
  • AbstractReferenceCollection
  • ReferenceCollection
  • SyncReferenceCollection

Lack of documentation, especially for non-trivial but pervasively used types is, IMO, a hindrance to the widespread adoption of the platform. Imagine STL or any other major library expecting developers to learn and understand its various facilities by reading the code line-by-line. It significantly increases the learning curve and makes the platform less approachable.

Where I am having to do such line-by-line studying of the code to understand these essential utilties, I'll be making contributions to hopefully bring some relief. However, I do think this technical debt should be tackled in a more comprehensive and intentional manner now that Theia has presumably reached some sort of critical mass.

How to test

Someone intimate with these utility types should read the new documentation and check for accuracy and understandability.

Follow-ups

N/A

Review checklist

Reminder for reviewers

   DisposableCollection
   AbstractReferenceCollection
   ReferenceCollection
   SyncReferenceCollection

Lack of documentation, especially for non-trivial but pervasively used
types is, IMO, a hindrance to the widespread adoption of the platform.
Imagine STL or any other major library expecting developers to learn
and understand its various facilities by reading the code line-by-line.
It significantly increases the learning curve and makes the platform
less approachable.

Where I am having to do such line-by-line studying of the code to
understand these essential utilties, I'll be making contributions to
hopefully bring some relief. However, I do think this technical debt
should be tackled in a more comprehensive and intentional manner now
that Theia has presumably reached some sort of critical mass.
@jcortell68 jcortell68 changed the title Adds comment-documentation for several key Theia utility classes Add comment-documentation for several key Theia utility classes Jan 29, 2024
@martin-fleck-at martin-fleck-at self-requested a review January 31, 2024 14:38
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you very much for the additional documentation, John! I do have some remarks regarding the DisposableCollection but other than that, this looks correct to me.

packages/core/src/common/disposable.ts Outdated Show resolved Hide resolved
packages/core/src/common/disposable.ts Show resolved Hide resolved
packages/core/src/common/disposable.ts Outdated Show resolved Hide resolved
packages/core/src/common/disposable.ts Outdated Show resolved Hide resolved
packages/core/src/common/disposable.ts Show resolved Hide resolved
@jcortell68 jcortell68 force-pushed the jcortell_doc1 branch 3 times, most recently from a40064a to 9655e9b Compare February 2, 2024 13:22
Improved description based on Martin's feedback.
@jcortell68
Copy link
Contributor Author

@martin-fleck-at Sorry for the churn. All done now and ready for re-review when you get a chance.

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you for the quick update, John! I think we are almost there, I'd just like some cleanup of the new test case.

packages/core/src/common/disposable.ts Outdated Show resolved Hide resolved
packages/core/src/common/disposable.spec.ts Outdated Show resolved Hide resolved
@jcortell68
Copy link
Contributor Author

@martin-fleck-at I think I took care of all your feedback items. Let me know if I missed something

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you for the quick update, everything looks good to me now!

@martin-fleck-at martin-fleck-at merged commit 466b16d into eclipse-theia:master Feb 5, 2024
14 checks passed
@jcortell68 jcortell68 deleted the jcortell_doc1 branch February 5, 2024 13:30
@jfaltermeier jfaltermeier added this to the 1.47.0 milestone Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants