Skip to content

Conversation

roji
Copy link
Member

@roji roji commented Nov 4, 2019

Inspected the source code, and it seems like there's a pattern in ODBC where a large buffer is assigned to a parameter, and then Offset and Size are used to refer to a window within that buffer each time (see this doc page).

/cc @carlossanlop @mairaw @ajcvickers

@roji roji requested a review from stevestein as a code owner November 4, 2019 11:53
@carlossanlop carlossanlop added 🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Nov 4, 2019
@carlossanlop carlossanlop added this to the November 2019 milestone Nov 4, 2019
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Looks great, @roji. Thanks for making the change. Let's wait for the build to finish without warnings, and also for a review from the @dotnet/docs team.

Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Left a comment for you to consider, @roji.

@carlossanlop carlossanlop removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Nov 4, 2019
@roji
Copy link
Member Author

roji commented Nov 4, 2019

Build passed

Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

@roji the build had another warning (it will say it passes, but we need to look at the "warning" message in the CI).
I left a comment. Can you please address it?

@roji roji force-pushed the OdbcParameterOffset branch from 2fec758 to 7da208e Compare November 4, 2019 18:42
Copy link
Contributor

@carlossanlop carlossanlop 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 @roji. Approved.
@dotnet/docs can someone from the Docs team approve and merge this change?

@Thraka
Copy link
Contributor

Thraka commented Nov 4, 2019

@carlossanlop

Sure thing. Question about it though. Are you all using a tool to import these comments? I continually see these weird indentation on the markdown remarks. I don't know why it's all indented 1 space under the heading, but it's badly formed markdown.

@roji
Copy link
Member Author

roji commented Nov 4, 2019

I personally copy-paste other remark fragments and try to maintain whatever style/indentation is already there... we may want to do a pass over whole sections to correct this kind of thing.

@carlossanlop
Copy link
Contributor

@Thraka this PR in particular was manually documented by @roji .

I do have a tool that automatically ports triple slash comments to dotnet-api-docs, and I have a few PRs out that were generated with it (any PR I have for System.Reflection.Metadata). No one else uses this tool (except for one isolated PR sent a few months ago by the WPF team, but it has been since merged).

All my other PRs I have out for review were manually documented by me, no tool involved.

Can you point out the indentation problems you're referring to? Here or in my PRs.

@Thraka
Copy link
Contributor

Thraka commented Nov 4, 2019

Pretty much all of them (and I think they are generated by the tools) have a starting space on all lines under ## Remarks. To pass generic linter rules, you want empty lines between types of content (headers, lists, paragraphs, etc) and don't want lines to start with a space.

so in this example, you would have something like:

          <format type="text/markdown"><![CDATA[

## Remarks
+
- The <xref:System.Data.Odbc.OdbcParameter.Offset> property is used for binary and string types. For fixed length data types, the value of <xref:System.Data.Odbc.OdbcParameter.Offset> is ignored.
+The <xref:System.Data.Odbc.OdbcParameter.Offset> property is used for binary and string types. For fixed length data types, the value of <xref:System.Data.Odbc.OdbcParameter.Offset> is ignored.

- For nonstring data types and ANSI string data, the <xref:System.Data.Odbc.OdbcParameter.Offset> property refers to the number of bytes. For Unicode string data, <xref:System.Data.Odbc.OdbcParameter.Offset> refers to the number of characters.
+For nonstring data types and ANSI string data, the <xref:System.Data.Odbc.OdbcParameter.Offset> property refers to the number of bytes. For Unicode string data, <xref:System.Data.Odbc.OdbcParameter.Offset> refers to the number of characters.

- ]]></format>
+]]></format>

@Thraka Thraka merged commit 842b647 into dotnet:master Nov 4, 2019
@roji roji deleted the OdbcParameterOffset branch November 7, 2019 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases new-content Indicates PRs that contain new articles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants