-
Notifications
You must be signed in to change notification settings - Fork 203
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
Code cleanup #325
Code cleanup #325
Conversation
sungam3r
commented
Jan 14, 2024
•
edited
Loading
edited
Some of these policies I'm going to disagree with because they match the internal coding styles we use. For example, we require use of |
# Conflicts: # UnitTests/UnitTests.csproj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review again once some of these questions and issues are resolved.
.editorconfig
Outdated
dotnet_style_qualification_for_property = false:warning | ||
dotnet_style_qualification_for_method = false:warning | ||
dotnet_style_qualification_for_event = false:warning | ||
csharp_style_namespace_declarations = file_scoped:suggestion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our internal policy is block_scoped, so I would prefer that still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
BenchmarkTests/combine-bechmarks.csx
Outdated
@@ -10,20 +10,20 @@ var resultsPath = Path.Combine(resultsDir, resultsFile + ".json"); | |||
|
|||
if (!Directory.Exists(resultsDir)) | |||
{ | |||
throw new DirectoryNotFoundException($"Directory not found '{resultsDir}'"); | |||
throw new DirectoryNotFoundException($"Directory not found '{resultsDir}'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are lots of spacing changes. What operation are you doing here, exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed indentation from tabs to spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah....I'm actually surprised I didn't notice that. My IDE is set to spaces. Reasonable, thanks!
using (var stream = this.manager.GetStream("Program.Main", this.sourceBuffer, | ||
0, this.sourceBuffer.Length)) |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
stream
.editorconfig
Outdated
dotnet_style_qualification_for_field = true:warning | ||
dotnet_style_qualification_for_property = false:warning | ||
dotnet_style_qualification_for_method = false:warning | ||
dotnet_style_qualification_for_event = false:warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I wasn't clear earlier. These should all be true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done