Skip to content

Conversation

@CorruptComputer
Copy link
Contributor

Summary

  • Adds csproj file for String C# code samples
  • Cleans up some of the code samples to be more readable

Still to do:

  • String.IndexOfAny()
  • String.Intern()
  • String.IsInterned()
  • String.IsNullOrEmpty()
  • String.Join()
  • String.LastIndexOf()
  • String.Length
  • String.Normalize()
  • String.Replace()
  • String.ToCharArray()
  • String.ToLower()
  • String.ToUpper()

@opbld32
Copy link

opbld32 commented Oct 13, 2021

Docs Build status updates of commit be98c3a:

✅ Validation status: passed

File Status Preview URL Details
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Compare/StringCompareSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Concat/StringConcatSamples.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Contains/StringContainsSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Equals/StringEqualsSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/GetTypeCode/StringGetTypeCodeSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/IndexOf/StringIndexOfSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Program.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Remove/StringRemoveSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/StringSamples.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.comp4/CS/string.comp4.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.compare3/CS/comp3.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.compare4/CS/comp4.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.compare5/CS/comp5.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.compareordinal/CS/comp0.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.concat5/CS/string.concat5.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.contains/CS/cont.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.equals/CS/equals.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.gettypecode/CS/gtc.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.indexof1/CS/ixof1.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.indexof8/CS/ixof8.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.remove/CS/r.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/stringremove/CS/stringremove.cs ✅Succeeded
xml/System/String.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

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 so much for helping with this! I left some comments for you to consider. Some of them are mandatory, and I also have some optional requests, if you'd like to help with that.

@@ -0,0 +1,171 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in all the source code files: This comment is mandatory for code snippets so they render correctly:

Suggested change
using System;
// <snippet1>
using System;

Copy link
Contributor

@adegeo adegeo Oct 13, 2021

Choose a reason for hiding this comment

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

Something may be out of sync here since the file has 5 snippets yet only #1 is being referenced in the xml, multiple times.

Also, is the new snippet going to be showing the whole entire file as you're suggesting @carlossanlop ? Because this file looks like it has multiple examples, each doing something slightly different. Also, snippet1 already exists in the article so your suggestion would make two of the same snippet. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I may have a wrong understanding of how <snippet1> works, and how it related to the id="Snippet1".

  1. I was under the impression that snippets should show fully working source code, not just an isolated method.
  2. The methods are called Sample1, Sample2, etc. Do these get translated to Snippet1, Snippet2, etc. somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I could understand reading the website and the source code, it uses the // <snippet1> as a sort of label to find which snippet of code you would like to reference from the file. I don't think the name of the method has any bearing on it, though I'm not 100% sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as the API system is working the same as any other normal conceptual article, if the id is omitted, the whole file is shown. If id="value" it will show only the code between //<value> and //</value> Method names and anything else in the article aren't automatically mapped in any way.

Console.WriteLine();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in all the source code files: This comment is mandatory for snippets to render correctly:

Suggested change
}
}
// </snippet1>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each code sample method should have their own // <snippetX> and // </snippetX> at the start and ends, is this required for the whole file as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not required for the whole file.


:::code language="cpp" source="~/samples/snippets/cpp/VS_Snippets_CLR/string.comp4/CPP/string.comp4.cpp" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/string.comp4/CS/string.comp4.cs" interactive="try-dotnet" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/String/CS/Compare/StringCompareSamples.cs" interactive="try-dotnet-method" id="Snippet1":::
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere: The try-dotnet string should not be modified. Can you please revert it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong though. @gewarren can you please confirm?

Copy link
Contributor Author

@CorruptComputer CorruptComputer Oct 13, 2021

Choose a reason for hiding this comment

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

From what I could tell by switching from the local copy to the website, if interactive="try-dotnet-method" is done then the code can run without the extra class info like the usings, namespace, and method, but if done as interactive="try-dotnet" instead it requires the full code of the class. I had made this change to hopefully make some of these snippets a bit shorter.

Example for interactive="try-dotnet-method".

Example for interactive="try-dotnet".

If this is not the case, please let me know and I can change them back.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mairaw Can probably advise.

Copy link
Contributor

Choose a reason for hiding this comment

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

The assumption is correct. But one thing I always tried to do was to have matching samples for C++ and VB too. So if the C# is shorter, the other ones should be too, no?


:::code language="cpp" source="~/samples/snippets/cpp/VS_Snippets_CLR/string.compare3/CPP/comp3.cpp" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/string.compare3/CS/comp3.cs" interactive="try-dotnet-method" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/String/CS/Compare/StringCompareSamples.cs" interactive="try-dotnet-method" id="Snippet2":::
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere: The id="SnippetX" string should not be modified. Notice the line above has a C++ sample, and below there's a Visual Basic sample. They should all have the exact same name.

Copy link
Contributor

Choose a reason for hiding this comment

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

@adegeo is this assumption correct? The id should match for all the different languages?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it really matters, as long as it's the right snippet.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't matter but having different file structures and snippet ids makes it harder to mantain IMHO.


:::code language="cpp" source="~/samples/snippets/cpp/VS_Snippets_CLR/string.compare4/CPP/comp4.cpp" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/string.compare4/CS/comp4.cs" interactive="try-dotnet-method" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/String/CS/Compare/StringCompareSamples.cs" interactive="try-dotnet-method" id="Snippet3":::
Copy link
Contributor

@carlossanlop carlossanlop Oct 13, 2021

Choose a reason for hiding this comment

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

I noticed you're renaming the C# files. That is fine, but please make sure you don't modify the behavior compared to the C++ and VB snippets. They should have the exact same behavior, and the only difference should be the languaged used.

But if you modify behavior, please do it for all languages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of the behavior will be changing, I've made sure to keep each of these methods working the same, and using the same calls to the related string methods. The only changes being made are purely cosmetic to make the code a bit easier to read.


:::code language="cpp" source="~/samples/snippets/cpp/VS_Snippets_CLR/string.compare3/CPP/comp3.cpp" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/string.compare3/CS/comp3.cs" interactive="try-dotnet-method" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/String/CS/Compare/StringCompareSamples.cs" interactive="try-dotnet-method" id="Snippet2":::
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be really awesome if you could move all the String C# samples out of the ugly folder called samples/snippets/csharp/VS_Snippets_CLR/, and move them to a new folder called: samples/snippets/csharp/System/String/, including the csproj. We want to slowly make it easier for maintainers to find their samples by searching for the assembly name, then the type name.

Do you think you can do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I would be happy to make that change! I had kept them under the folder VS_Snippets_CLR since it seemed to be a convention, but if you want to move them then for sure I agree it would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Example of previous code that we have moved to prettier folders:

#7265
#7267
#7289

:::code language="cpp" source="~/samples/snippets/cpp/VS_Snippets_CLR/string.compare3/CPP/comp3.cpp" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/string.compare3/CS/comp3.cs" interactive="try-dotnet-method" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/String/CS/Compare/StringCompareSamples.cs" interactive="try-dotnet-method" id="Snippet2":::
:::code language="vb" source="~/samples/snippets/visualbasic/VS_Snippets_CLR/string.compare3/VB/comp3.vb" id="Snippet1":::
Copy link
Contributor

@carlossanlop carlossanlop Oct 13, 2021

Choose a reason for hiding this comment

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

Totally optional: If you have time and interest, can you do the same for the VB and C++ examples?

If you feel like the C# samples need to be modified, and you want to make sure the VB samples do the same work, you can use the Telerik C# to VB translator to help get the VB version of your C# code faster. It's not perfect, but it's great for these types of tasks.

I don't think there's a C# to C++ translator (not that I know of). So they would need manual modification, so the behavior matches that of C#.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, and also totally optional, if you don't plan on modifying the VB and C++, please move the VB and C++ to nicer folders:

  • samples/snippets/cpp/System/String/
  • samples/snippets/vb/System/String/

Note that if you decide to move them, they'll also need a new project file added, otherwise the CI will fail like in your other PR because they were touched.

@carlossanlop
Copy link
Contributor

@CorruptComputer After some of my suggestions get addressed, maybe the failures go away with a new CI run. If not, then we can take a deeper look.

@gewarren FYI the "Snippets 5000" CI leg failed with an unhelpful error message:

GitHub Actions has encountered an internal error when running your job.

@carlossanlop carlossanlop requested a review from gewarren October 13, 2021 18:57
@gewarren
Copy link
Contributor

@adegeo For the Snippets 5000 error message.


:::code language="cpp" source="~/samples/snippets/cpp/VS_Snippets_CLR/string.compare5/CPP/comp5.cpp" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/string.compare5/CS/comp5.cs" id="Snippet1":::
:::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR/String/CS/Compare/StringCompareSamples.cs" interactive="try-dotnet-method" id="Snippet4":::
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what @adegeo mentioned in this other comment: You're only referencing StringCompareSamples.cs in all the snippets you modified. None of the other cs files are being referenced.

Copy link
Contributor Author

@CorruptComputer CorruptComputer Oct 13, 2021

Choose a reason for hiding this comment

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

Ah yeah you are right, I changed some of these to the wrong file/snippet. Those should be fixed now, thanks for the heads up!

@CorruptComputer
Copy link
Contributor Author

CorruptComputer commented Oct 13, 2021

What I may end up doing to hopefully reduce errors and for easier review, is splitting this up into multiple small PR's. Would that be preferable over a large single PR?

@opbld34
Copy link

opbld34 commented Oct 13, 2021

Docs Build status updates of commit 52274f2:

✅ Validation status: passed

File Status Preview URL Details
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Compare/StringCompareSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Concat/StringConcatSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Contains/StringContainsSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Equals/StringEqualsSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/GetTypeCode/StringGetTypeCodeSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/IndexOf/StringIndexOfSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Program.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/String/CS/Remove/StringRemoveSamples.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/String/CS/StringSamples.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.comp4/CS/string.comp4.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.compare3/CS/comp3.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.compare4/CS/comp4.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.compare5/CS/comp5.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.compareordinal/CS/comp0.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.concat5/CS/string.concat5.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.contains/CS/cont.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.equals/CS/equals.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.gettypecode/CS/gtc.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.indexof1/CS/ixof1.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.indexof8/CS/ixof8.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/string.remove/CS/r.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/stringremove/CS/stringremove.cs ✅Succeeded
xml/System/String.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@gewarren
Copy link
Contributor

gewarren commented Feb 2, 2022

What I may end up doing to hopefully reduce errors and for easier review, is splitting this up into multiple small PR's. Would that be preferable over a large single PR?

Sorry that this went unanswered. Yes, multiple PRs would be easier to review. Also, all of the snippets in the VS_Snippets_CLR folder have now been reorganized in the way that Carlos suggested, and the top-level samples folder was removed, so your PR probably won't be quite as big once you pull in those changes.

@gewarren
Copy link
Contributor

gewarren commented Apr 1, 2022

@CorruptComputer Do you still plan to work on this? I suggest we close this PR and you can add the readability changes on a new branch that's up to date with main (we made a lot of changes in the snippet folder structure).

@CorruptComputer
Copy link
Contributor Author

@gewarren Sorry I've been quite busy lately and haven't found the time to come back to work on this. I'll just close this for now.

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.

7 participants