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

Fix a couple of typos in Jsrt Header comments #5659

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented Sep 1, 2018

This PR fixes a few typos I noticed in the Jsrt Header comments as well as one pointed out by @RealTrisT in #5615

@@ -199,7 +199,7 @@ typedef unsigned short uint16_t;
/// </summary>
JsErrorInObjectBeforeCollectCallback,
/// <summary>
/// Object cannot be unwrapped to IInspectable pointer.
/// Object cannot be unwrapped to Inspectable pointer.
Copy link
Contributor

Choose a reason for hiding this comment

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

This might have been intentional - IInspectable by .NET convention would be Interface Inspectable

Copy link
Contributor

Choose a reason for hiding this comment

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

.NET and, possibly more importantly, WinRT convention. I think this has to do with JSRT Projection

@@ -237,6 +237,7 @@ typedef unsigned short uint16_t;
/// Module was not yet evaluated when JsGetModuleNamespace was called.
/// </summary>
JsErrorModuleNotEvaluated,

Copy link
Contributor

Choose a reason for hiding this comment

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

why add this blank line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The next line is the start of a category, all the other categories are preceded by a blank line.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

Copy link
Contributor

@dilijev dilijev left a comment

Choose a reason for hiding this comment

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

Looks fine, but needs a review from someone familiar with the conventions. /cc @liminzhu

We should propagate the changes to these comments over to the wiki -- you can send a PR to https://github.com/Microsoft/ChakraCore-wiki

@jackhorton
Copy link
Contributor

Shouldn't the wiki be auto-generate-able from the header? Id imagine we already do that for the MSDN page.

Copy link
Collaborator

@liminzhu liminzhu left a comment

Choose a reason for hiding this comment

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

Thanks for catching those!

@@ -217,7 +217,7 @@ typedef unsigned short uint16_t;
/// </summary>
JsErrorInvalidContext,
/// <summary>
/// Module evaluation is called in wrong context.
/// Invalid Module HostInfoKind provided to JsSetModuleHostInfo or JsGetModuleHostInfo.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd avoid mentioning specific APIs for future-proofing and just say The Module HostInfoKind provided is invalid.

@chakrabot chakrabot merged commit 63ca567 into chakra-core:master Sep 4, 2018
chakrabot pushed a commit that referenced this pull request Sep 4, 2018
Merge pull request #5659 from rhuanjl:updateHeaders

This PR fixes a few typos I noticed in the Jsrt Header comments as well as one pointed out by @RealTrisT in #5615
@rhuanjl rhuanjl deleted the updateHeaders branch September 4, 2018 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants