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

chore: add comments documenting public API #83

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

abhishekgite446
Copy link

Addressing Issue 76

Adding comment using JSDoc for the function runFromCommandLine and replacing undocumented comments in etc/fiddle-core.api.md using yarn docs

@abhishekgite446 abhishekgite446 requested a review from a team as a code owner June 23, 2023 16:25
@abhishekgite446 abhishekgite446 changed the title Adding comment for the function runFromCommandLine docs: Adding comment for the function runFromCommandLine Jun 23, 2023
@abhishekgite446
Copy link
Author

abhishekgite446 commented Jun 23, 2023

Hi @dsanders11, apologies that I had to close my previous commit due to a git issue. Opening this new PR to address Issue 76. I have only update the description for the function runFromCommandLine for now, can you please have a look ?

@dsanders11
Copy link
Member

It seems unnecessary to bump the version of @microsoft/api-extractor? The changes to etc/fiddle-core.api.md are also not included in these changes (which causes the workflow failure). Also, the comment shouldn't mention runFromCommandLine, and doesn't currently pass lint.

@abhishekgite446
Copy link
Author

Thanks for having a look @dsanders11. I guess pushed the changes in a rush. Thanks a lot for spotting everything, I have addressed all the issues you have raised previously. Can you please have a look when you get time :)

@abhishekgite446
Copy link
Author

Just so that you are aware, the only reason I bumped the version for @microsoft/api-extractor because I saw the following message when I executed yarn docs. Nevertheless the version bump was not required in the first place.

@abhishekgite446 abhishekgite446 changed the title docs: Adding comment for the function runFromCommandLine docs: Adding comment in order to remove the undocumented from .md file Jun 24, 2023
@abhishekgite446
Copy link
Author

@dsanders11 when you get time can you please have a look :)

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

The changes in yarn.lock are leftover from the previous package.json change and need to be dropped from this PR.

src/installer.ts Outdated Show resolved Hide resolved
src/paths.ts Outdated Show resolved Hide resolved
src/versions.ts Outdated Show resolved Hide resolved
src/command-line.ts Outdated Show resolved Hide resolved
src/fiddle.ts Outdated Show resolved Hide resolved
src/fiddle.ts Outdated Show resolved Hide resolved
@dsanders11 dsanders11 changed the title docs: Adding comment in order to remove the undocumented from .md file chore: add comments documenting public API Jun 29, 2023
@abhishekgite446
Copy link
Author

@dsanders11 as suggested made all changes. Please have a look at your convenience.

Copy link
Member

@dsanders11 dsanders11 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 avoid starting the comments with wordings like "Represents", "Overrides", "This method", "This object" and "This function". I'd say avoid the usage of "represents" all together, similarly with "it" and "this" when possible.

For getter methods (like those in src/versions.ts) drop the starting "Gets", document it like it's a variable, rather than a method.

src/fiddle.ts Outdated Show resolved Hide resolved
src/fiddle.ts Outdated Show resolved Hide resolved
src/installer.ts Outdated Show resolved Hide resolved
@abhishekgite446
Copy link
Author

@dsanders11 as suggested I have amended the comments, please have a look at your convenience 👍

@dsanders11 dsanders11 self-assigned this Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants