Skip to content

Review TODO code snippet references (docs) #1982

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

Merged
merged 6 commits into from
Apr 25, 2017

Conversation

tompratt-AQ
Copy link
Contributor

Title

Review TODO code snippet references and resolve them.

Summary

This PR covers all of the 25 conceptual files (\docs) (48 instances) that are listed in the dotnet/docs Project "TODO: review snippet reference".

Suggested Reviewers

@mairaw

@tompratt-AQ
Copy link
Contributor Author

The following comments apply to files that were edited in this PR:

docs/framework/winforms/controls/how-to-access-the-managed-html-document-object-model.md
Snippet doesn't exist and wasn't included in VS 2010.

docs/framework/wcf/how-to-specify-a-client-binding-in-configuration.md
Looks good in preview but some snippets aren't currently rendering.

docs/framework/wcf/feature-details/create-an-ajax-wcf-asp-net-client.md
docs/framework/wcf/feature-details/create-a-service-arbitrary-data-using-wcf.md
In both of these files, I deleted Registry Key#4, because I couldn't find the file and there is no mention of it in the topic or examples.

docs/framework/wcf/how-to-secure-a-service-with-windows-credentials.md
All of the C# snippets have a corresponding VB snippet, except for #2.

docs/framework/wcf/extending/how-to-inspect-and-modify-messages-on-the-service.md
I kept only the one config file, but C# and VB were slightly different than it. Do they all need to be kept because of that?

docs/framework/wcf/creating-ws-i-basic-profile-1-1-interoperable-services.md
Only one config file now, so I changed the intro to the snippet from plural to singular.

docs/framework/interop/platform-invoke-examples.md
The C++ snippet exists in the current version, but was TODO here. I kept it - is that correct?

For all of these files, I added %20 to the directory name in all snippet links because the paths wouldn't resolve otherwise:
docs/framework/data/adonet/query-expression-syntax-examples-restriction-linq-to-dataset.md
docs/framework/data/adonet/single-table-queries-linq-to-dataset.md
docs/framework/data/adonet/method-based-query-syntax-examples-element-operators.md
docs/framework/data/adonet/ado-net-code-examples.md
In this last file, 2 snippets don't exist anymore but were in VS 2010. I left a TODO message to that effect in case they can be found.

@rpetrusha
Copy link
Contributor

@tompratt-AQ, note that there are publishing errors.

@rpetrusha
Copy link
Contributor

@tompratt-AQ This looks good, aside from whatever build errors there may be. Assuming that you can fix them, I'll approve the PR.

@tompratt-AQ
Copy link
Contributor Author

Thanks. I'll work on these build errors. I'm not too surprised by them, given that I'm trying to resolve previously broken snippet links.

@mairaw
Copy link
Contributor

mairaw commented Apr 21, 2017

I agree with @rpetrusha. We can also start renaming some samples if it makes it easier to link to them (for example, to not have spaces in the file name). We just need to make sure to rename where they're referenced if you do that.

@tompratt-AQ
Copy link
Contributor Author

It's not really the file name in this case, it's the folder name.

For example, this path apparently builds but does not render the snippet on our doc page (“DataWorks SampleApp.OleDb” is the directory with the space):
[!code-csharp[DataWorks SampleApp.OleDb#1](../../../../samples/snippets/csharp/VS_Snippets_ADO.NET/DataWorks SampleApp.OleDb/CS/source.cs#1)]

I noticed that the path would resolve in my VS Code preview when I added%20, like this:
[!code-csharpDataWorks SampleApp.OleDb#1]

However, that causes a build error for “unable to resolve….cannot find reference”

Any ideas?

@mairaw mairaw added the WIP label Apr 21, 2017
@tompratt-AQ
Copy link
Contributor Author

Resolved all of the build errors that I could, and put back the TODO tags on snippets that still have errors due to a bug.

@tompratt-AQ
Copy link
Contributor Author

Added too many spaces to a few links, and missed some link breaks.

Copy link
Contributor

@mairaw mairaw 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 overall! Left some comments.

@@ -253,9 +254,9 @@ Class EntityClientSample
End Class
```

In C#"
In C#:
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need this anymore.

@@ -338,7 +339,7 @@ End Class

In C#:
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -24,7 +24,7 @@ The examples in this topic demonstrate how to use the <xref:System.Linq.Enumerab

The examples in this topic use the following `using`/`Imports` statements:

<!-- TODO: review snippet reference [!CODE [DP LINQ to DataSetExamples#ImportsUsing](DP LINQ to DataSetExamples#ImportsUsing)] -->
[!code-csharp[DP LINQ to DataSetExamples#ImportsUsing](../../../../samples/snippets/csharp/VS_Snippets_ADO.NET/DP LINQ to DataSet Examples/CS/Program.cs#importsusing)]
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also the VB version for these. Please add to all the LINQ to DataSet Examples references.

@tompratt-AQ
Copy link
Contributor Author

Thanks for the helpful review comments!
I made the fixes as suggested and added one additional file with several changes. There are now 22 Docs files (conceptual topics) remaining from the list. I will submit them in a separate PR before I start in on the XML files.

Copy link
Contributor

@mairaw mairaw 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 @tompratt-AQ! Let's merge this.

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.

4 participants