Skip to content

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Aug 2, 2020

This PR just runs dotnet format . --folder command in the repository root.

The only concern is if a file is referenced by line number, so please hold off on merging this until I try to see a solution to make sure that none of these are accessed by line.

This is a list of the changed files in the PR.

editorconfig needs a very careful review, as the dotnet format tool relies on it.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Aug 2, 2020

Before I proceed with checking access-by-line.txt, I'll wait for a feedback whether you can accept this PR or you want to take a smaller PR (an option for taking a smaller PRs could be to use specific editorconfig options at first).


This is a list of all the access-by-line.txt files in the repository:

docs\core\deploying\snippets\deploy-with-vs\xml\access-by-line.txt
docs\csharp\programming-guide\concepts\async\snippets\index\AsyncBreakfast-final\access-by-line.txt
docs\csharp\programming-guide\concepts\async\snippets\index\AsyncBreakfast-starter\access-by-line.txt
samples\snippets\csharp\snippets\tour\access-by-line.txt
samples\snippets\csharp\tour\arrays\access-by-line.txt
samples\snippets\csharp\tour\attributes\access-by-line.txt
samples\snippets\csharp\tour\classes-and-objects\access-by-line.txt
samples\snippets\csharp\tour\delegates\access-by-line.txt
samples\snippets\csharp\tour\interfaces\access-by-line.txt
samples\snippets\csharp\tour\program-structure\access-by-line.txt
samples\snippets\csharp\tour\statements\access-by-line.txt
samples\snippets\csharp\tour\types-and-variables\access-by-line.txt
samples\snippets\csharp\VS_Snippets_VBCSharp\VbRadconService\CS\access-by-line.txt
samples\snippets\csharp\VS_Snippets_Wpf\ApplicationStartupSnippets\CSharp\access-by-line.txt
samples\snippets\csharp\VS_Snippets_Wpf\BusinessLayerValidation\CSharp\access-by-line.txt
samples\snippets\csharp\VS_Snippets_Wpf\DialogBoxSample\CSharp\access-by-line.txt
samples\snippets\csharp\VS_Snippets_Wpf\ExpenseIt\CSharp\ExpenseIt8\access-by-line.txt
samples\snippets\csharp\VS_Snippets_Wpf\ExpenseIt\CSharp\ExpenseIt9\access-by-line.txt
samples\snippets\csharp\VS_Snippets_Wpf\HOWTOApplicationModelSnippets\CSharp\access-by-line.txt
samples\snippets\csharp\VS_Snippets_Wpf\SimpleBinding\CSharp\access-by-line.txt
samples\snippets\fsharp\access-by-line.txt
samples\snippets\fsharp\azure\access-by-line.txt
samples\snippets\fsharp\getting-started\access-by-line.txt
samples\snippets\fsharp\tuples\access-by-line.txt
snippets\visualbasic\language-reference\error-messages\bc30451\access-by-line.txt
samples\snippets\visualbasic\language-reference\error-messages\bc30456\access-by-line.txt
samples\snippets\visualbasic\VS_Snippets_VBCSharp\VbRadconService\VB\access-by-line.txt
samples\snippets\visualbasic\VS_Snippets_Wpf\ApplicationStartupSnippets\visualbasic\access-by-line.txt
samples\snippets\visualbasic\VS_Snippets_Wpf\DialogBoxSample\VisualBasic\access-by-line.txt
samples\snippets\visualbasic\VS_Snippets_Wpf\HOWTOApplicationModelSnippets\visualbasic\access-by-line.txt
samples\snippets\visualbasic\VS_Snippets_Wpf\SimpleBinding\VisualBasic\access-by-line.txt

I've manually checked until "samples\snippets\csharp\tour\types-and-variables\access-by-line.txt", and fixed some line numbering issues.

An automated solution would be much easier than checking these manually.

Comment on lines +35 to +39
//(used if a resource is not found in the page,
// or application resource dictionaries)
ResourceDictionaryLocation.SourceAssembly //where the generic resource dictionary is located
//(used if a resource is not found in the page,
// app, or any theme specific resource dictionaries)
//(used if a resource is not found in the page,
// app, or any theme specific resource dictionaries)
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this intended? What option in editorconfig can control this?

@Youssef1313
Copy link
Member Author

I've manually reviewed that no other line numbering has changed.

@tdykstra
Copy link
Contributor

tdykstra commented Aug 3, 2020

@Youssef1313 Thanks for your efforts on this, but the team prefers not to accept bulk change PRs without a demonstrable need. That applies especially to code files when there are too many to review, let alone test. A problem for this PR in particular is the fact that some snippets may not follow editorconfig by design. Also, access-by-line.txt files are not a reliable way to find every instance where line numbers are in fact referenced; a file's presence alerts you to line number use but its absence doesn't guarantee the absence of line number use. Even if we could invest the amount of effort required to review these changes thoroughly enough, the limited benefit isn't enough to make that work the best use of the limited resources we have available.

To avoid wasted effort, in the future when you have an idea that will take a significant amount of time to implement, please start by creating an issue to find out if it's going to be something the team agrees is needed and worth the time to review related PRs.

@tdykstra tdykstra closed this Aug 3, 2020
@Youssef1313
Copy link
Member Author

Youssef1313 commented Aug 4, 2020

@tdykstra, There are two options in editorconfig that will never cause any problems (doesn't affect line numbers, and will never be made intentionally), and the fact of the huge files changed is actually caused by them.

They are:

charset = utf-8-bom
insert_final_newline = true

I'm thinking of two possibilities:

  1. Apply only these two in one PR to make sure nothing will break. Then after it's merged, see if the number of changed files will be decreased and if it's manageable to review or not.
  2. Disable these options from editorconfig before applying dotnet format, and directly check if the number of files will be manageable or not.

Pinging @sharwell for the utf-8-bom thing, I think he opened one PR before to add the bom char to all files in samples repo. I'm not sure why it might be important to use utf-8-bom.

@Youssef1313
Copy link
Member Author

dotnet/samples#1085
dotnet/samples#1091

These are @sharwell PRs I'm referring to for adding the BOM char

@sharwell
Copy link
Contributor

sharwell commented Aug 4, 2020

I'm not sure why it might be important to use utf-8-bom.

When the BOM is missing, some editors will improperly load file content under a different default encoding. The most common scenario where I have observed this is repositories containing author names in comments, and you can see over time many names have lost their original characters. utf-8-bom is the most consistently handled file encoding, covering all modern text editors both with and without support for .editorconfig, and is thus the least likely to produced invalid file content over time in a distributed team.

@tdykstra
Copy link
Contributor

tdykstra commented Aug 4, 2020

@sharwell What tool did you use to make your PRs in the samples repo?

@sharwell
Copy link
Contributor

sharwell commented Aug 4, 2020

I use a script, but then I manually verify that each character has the same interpretation under known default encodings, and verify correct interpretation for any differences that appear.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Aug 4, 2020

@sharwell, would it be okay if I ran dotnet format with only bom option?

I'm unsure of how you verify correct interpretation for any differences

@sharwell
Copy link
Contributor

sharwell commented Aug 4, 2020

@Youssef1313 I can run my script on this repository to see what condition it is currently in

@Youssef1313
Copy link
Member Author

@sharwell Great.

@sharwell
Copy link
Contributor

sharwell commented Aug 4, 2020

@Youssef1313 I filed #19825 with the changes

I verify correctness by reviewing each character change (under a binary diff) aside from the addition of the BOM itself.

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.

3 participants