-
Notifications
You must be signed in to change notification settings - Fork 33
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
NO MERGE - code quality analysis spike #463
base: main
Are you sure you want to change the base?
Changes from all commits
8b07548
99502e8
2f44128
13f4bf6
4cb0505
d996553
b72f0a7
37e09b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
# References | ||
# https://docs.microsoft.com/en-us/visualstudio/code-quality/use-roslyn-analyzers?view=vs-2022 | ||
# https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/language-rules | ||
# https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/naming-rules | ||
# https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ | ||
# https://rules.sonarsource.com/csharp | ||
# https://editorconfig.org/ | ||
|
||
root = true | ||
|
||
[**/*] | ||
# Sensible Defaults | ||
file_header_template = SPDX-License-Identifier: Apache-2.0\nLicensed to the Ed-Fi Alliance under one or more agreements.\nThe Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0.\nSee the LICENSE and NOTICES files in the project root for more information. | ||
indent_style=space | ||
indent_size=4 | ||
tab_width=2 | ||
insert_final_newline = true | ||
charset = utf-8 | ||
trim_trailing_whitespace = true | ||
max_line_length = 110 | ||
|
||
[**/**.min.js] | ||
# Minified JavaScript files shouldn't be changed | ||
indent_style = ignore | ||
insert_final_newline = ignore | ||
|
||
[**/*.md] | ||
trim_trailing_whitespace = false | ||
|
||
[**/*.{cs,cshtml,csx}] | ||
indent_style = space | ||
indent_size = 4 | ||
tab_width = 4 | ||
end_of_line = crlf | ||
csharp_new_line_before_members_in_object_initializers = false | ||
csharp_preferred_modifier_order = private, public, protected, internal, new, abstract, virtual, sealed, override, static, readonly, extern, unsafe, volatile, async:suggestion | ||
|
||
dotnet_diagnostic.IDE0073.severity=error # File header template required | ||
|
||
# Formatting rules | ||
dotnet_diagnostic.IDE0055.severity=warning | ||
csharp_new_line_before_open_brace = all # Allman style | ||
csharp_new_line_before_else = true | ||
csharp_new_line_before_catch = true | ||
csharp_new_line_before_finally = true | ||
csharp_new_line_before_members_in_object_initializers = true | ||
csharp_new_line_before_members_in_anonymous_types = true | ||
csharp_new_line_between_query_expression_clauses = true | ||
csharp_indent_case_contents = true | ||
csharp_indent_switch_labels = true | ||
csharp_indent_labels = flush_left | ||
csharp_indent_block_contents = true | ||
csharp_indent_braces = false | ||
csharp_indent_case_contents_when_block = true | ||
csharp_space_after_cast = true | ||
csharp_space_after_keywords_in_control_flow_statements = true | ||
csharp_space_between_parentheses = none | ||
csharp_space_before_colon_in_inheritance_clause = true | ||
csharp_space_after_colon_in_inheritance_clause = true | ||
csharp_space_around_binary_operators = before_and_after | ||
csharp_space_between_method_declaration_parameter_list_parentheses = false | ||
csharp_space_between_method_declaration_empty_parameter_list_parentheses = false | ||
csharp_space_between_method_declaration_name_and_open_parenthesis = false | ||
csharp_space_between_method_call_parameter_list_parentheses = false | ||
csharp_space_between_method_call_empty_parameter_list_parentheses = false | ||
csharp_space_between_method_call_name_and_opening_parenthesis = false | ||
csharp_space_after_comma = true | ||
csharp_space_before_comma = false | ||
csharp_space_after_dot = false | ||
csharp_space_before_dot = false | ||
csharp_space_after_semicolon_in_for_statement = true | ||
csharp_space_before_semicolon_in_for_statement = false | ||
csharp_space_around_declaration_statements = false | ||
csharp_space_before_open_square_brackets = false | ||
csharp_space_between_empty_square_brackets = false | ||
csharp_space_between_square_brackets = false | ||
|
||
# Naming rules | ||
dotnet_naming_rule.local_constants_rule.severity = warning | ||
dotnet_naming_rule.local_constants_rule.style = upper_camel_case_style | ||
dotnet_naming_rule.local_constants_rule.symbols = local_constants_symbols | ||
dotnet_naming_rule.private_constants_rule.import_to_resharper = as_predefined | ||
dotnet_naming_rule.private_constants_rule.severity = warning | ||
dotnet_naming_rule.private_constants_rule.style = upper_camel_case_style | ||
dotnet_naming_rule.private_constants_rule.symbols = private_constants_symbols | ||
dotnet_naming_rule.private_static_readonly_rule.import_to_resharper = as_predefined | ||
dotnet_naming_rule.private_static_readonly_rule.severity = warning | ||
dotnet_naming_rule.private_static_readonly_rule.style = lower_camel_case_style | ||
dotnet_naming_rule.private_static_readonly_rule.symbols = private_static_readonly_symbols | ||
dotnet_naming_style.lower_camel_case_style.capitalization = camel_case | ||
dotnet_naming_style.lower_camel_case_style.required_prefix = _ | ||
dotnet_naming_style.upper_camel_case_style.capitalization = pascal_case | ||
dotnet_naming_symbols.local_constants_symbols.applicable_accessibilities = * | ||
dotnet_naming_symbols.local_constants_symbols.applicable_kinds = local | ||
dotnet_naming_symbols.local_constants_symbols.required_modifiers = const | ||
dotnet_naming_symbols.private_constants_symbols.applicable_accessibilities = private | ||
dotnet_naming_symbols.private_constants_symbols.applicable_kinds = field | ||
dotnet_naming_symbols.private_constants_symbols.required_modifiers = const | ||
dotnet_naming_symbols.private_static_readonly_symbols.applicable_accessibilities = private | ||
dotnet_naming_symbols.private_static_readonly_symbols.applicable_kinds = field | ||
dotnet_naming_symbols.private_static_readonly_symbols.required_modifiers = static,readonly | ||
|
||
# Misc style | ||
dotnet_style_parentheses_in_arithmetic_binary_operators = never_if_unnecessary:none | ||
dotnet_style_parentheses_in_other_binary_operators = never_if_unnecessary:none | ||
dotnet_style_parentheses_in_relational_binary_operators = never_if_unnecessary:none | ||
dotnet_style_predefined_type_for_locals_parameters_members = true:suggestion | ||
dotnet_style_predefined_type_for_member_access = true:suggestion | ||
dotnet_style_qualification_for_event = false:suggestion | ||
dotnet_style_qualification_for_field = false:suggestion | ||
dotnet_style_qualification_for_method = false:suggestion | ||
dotnet_style_qualification_for_property = false:suggestion | ||
dotnet_style_require_accessibility_modifiers = for_non_interface_members:warning | ||
|
||
# We *like* var | ||
dotnet_diagnostic.IDE0008.severity=none # IDE0008: Use explicit type | ||
csharp_style_var_elsewhere = false:none | ||
csharp_style_var_for_built_in_types = false:suggestion | ||
|
||
# Using statements | ||
csharp_using_directive_placement=outside_namespace:warning | ||
dotnet_diagnostic.IDE0065.severity=warning # placement of using statements | ||
dotnet_diagnostic.IDE0005.severity=warning # Remove unnecessary using directives | ||
|
||
# Lower the priority | ||
dotnet_diagnostic.S4136.severity=suggestion # Method overloads should be grouped together | ||
dotnet_diagnostic.S1135.severity=suggestion # Complete TODO comments | ||
dotnet_diagnostic.S112.severity=suggestion # 'System.Exception' should not be thrown by user code. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lowered this from Warning to Suggestion |
||
|
||
|
||
[**/tests/**/*.cs] | ||
# Allow our strange test class naming convention | ||
dotnet_naming_symbols.amt_tests.applicable_kinds = class,method | ||
dotnet_naming_symbols.amt_tests.word_separator = "_" | ||
dotnet_naming_symbols.amt_tests.capitalization = first_word_upper | ||
dotnet_diagnostic.IDE1006.severity=none | ||
dotnet_diagnostic.S101.severity=none | ||
|
||
# SonarLint doesn't understand implied assertions from FakeItEasy | ||
dotnet_diagnostic.S2699.severity=none # S2699: Tests should include assertions |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,7 +64,7 @@ public async Task<ActionResult> Create(CreateLoginModel model) | |
} | ||
|
||
// Failed Model Validation | ||
Response.StatusCode = (int)HttpStatusCode.BadRequest; | ||
Response.StatusCode = (int) HttpStatusCode.BadRequest; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a mix of the two styles in our code base, and I had to choose one to enforce. Personally, I don't have a strong feeling about this, and I selected Microsoft's default option. |
||
return Json(BuildJsonResponseFromModelState()); | ||
} | ||
|
||
|
@@ -245,7 +245,7 @@ public ActionResult PasswordResetSuccessful() | |
return View( | ||
new PasswordResetCompletViewModel | ||
{ | ||
Message = (string)message | ||
Message = (string) message | ||
}); | ||
} | ||
|
||
|
@@ -296,7 +296,7 @@ public async Task<JsonResult> Login(LoginModel model) | |
} | ||
|
||
//Failed Login | ||
Response.StatusCode = (int)HttpStatusCode.Unauthorized; | ||
Response.StatusCode = (int) HttpStatusCode.Unauthorized; | ||
|
||
return Json( | ||
new | ||
|
@@ -307,7 +307,7 @@ public async Task<JsonResult> Login(LoginModel model) | |
} | ||
|
||
// Failed Model Validation | ||
Response.StatusCode = (int)HttpStatusCode.BadRequest; | ||
Response.StatusCode = (int) HttpStatusCode.BadRequest; | ||
return Json(BuildJsonResponseFromModelState()); | ||
} | ||
|
||
|
@@ -360,4 +360,4 @@ private string GetErrorMessageFromModelState() | |
return string.Join("\n", errorMessages); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// Licensed to the Ed-Fi Alliance under one or more agreements. | ||
// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0. | ||
// See the LICENSE and NOTICES files in the project root for more information. | ||
|
@@ -18,6 +18,7 @@ | |
using EdFi.Ods.Sandbox.Provisioners; | ||
using Microsoft.AspNetCore.Mvc; | ||
using Microsoft.AspNetCore.Authorization; | ||
using System.Text; | ||
|
||
namespace EdFi.Ods.SandboxAdmin.Controllers.Api | ||
{ | ||
|
@@ -92,12 +93,12 @@ public async Task<IActionResult> GetClients() | |
{ | ||
var currentException = e; | ||
var counter = 0; | ||
var message = string.Empty; | ||
var message = new StringBuilder(); | ||
|
||
while (currentException != null) | ||
{ | ||
Trace.TraceError("{0}: {1}", currentException.Message, currentException.StackTrace); | ||
message += string.Format("{0}:{1}", counter, currentException.Message); | ||
message.AppendLine($"{counter}:{currentException.Message}"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one of the code smells detected by the SonarSource analyzer. Arguably I should have kept it consistent with |
||
counter++; | ||
currentException = currentException.InnerException; | ||
} | ||
|
@@ -170,7 +171,7 @@ public async Task<IActionResult> PostClient([FromBody] SandboxClientCreateModel | |
} | ||
} | ||
|
||
return StatusCode((int)HttpStatusCode.NotAcceptable); | ||
return StatusCode((int) HttpStatusCode.NotAcceptable); | ||
} | ||
|
||
[HttpPut("{id}")] | ||
|
@@ -207,4 +208,4 @@ public void DeleteClient([FromRoute] string id) | |
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// Licensed to the Ed-Fi Alliance under one or more agreements. | ||
// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0. | ||
// See the LICENSE and NOTICES files in the project root for more information. | ||
|
@@ -10,7 +10,9 @@ | |
|
||
namespace EdFi.Ods.SandboxAdmin.Controllers.Api | ||
{ | ||
#pragma warning disable S101 // Types should be named in PascalCase, however, uppercase is preferred for acronyms such as "DTO" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may be able to go a step further and recognize DTO as legitimate. https://docs.microsoft.com/en-us/visualstudio/code-quality/how-to-customize-the-code-analysis-dictionary?view=vs-2022 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I dislike the "DTO" suffix entirely. I generally find it to be an unnecessary addition to the name. |
||
public class SandboxDTO | ||
#pragma warning restore S101 // Types should be named in PascalCase | ||
{ | ||
public string Command { get; set; } | ||
} | ||
|
@@ -41,4 +43,4 @@ public void Put(SandboxDTO sandboxDTO) | |
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// Licensed to the Ed-Fi Alliance under one or more agreements. | ||
// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0. | ||
// See the LICENSE and NOTICES files in the project root for more information. | ||
|
@@ -39,24 +39,6 @@ public IActionResult Get(int id) | |
return Ok(user); | ||
} | ||
|
||
//// POST api/userprofile | ||
//public void Post([FromBody]UserProfile value) | ||
//{ | ||
//} | ||
|
||
// PUT api/userprofile/5 | ||
/// <summary> | ||
/// Used to enable or disable user, nothing else | ||
/// </summary> | ||
/// <param name="id"></param> | ||
/// <param name="value"></param> | ||
|
||
//public void Put(int id, [FromBody]UserProfile value) | ||
//{ | ||
// var user = _clientAppRepo.GetUserProfile(id); | ||
// if (user == null) throw new HttpResponseException(HttpStatusCode.NotFound); | ||
//} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that this tooling helped find and remove dead code. |
||
// DELETE api/userprofile/5 | ||
public void Delete(int id) | ||
{ | ||
|
@@ -70,4 +52,4 @@ public void Delete(int id) | |
_clientAppRepo.DeleteUser(user); | ||
} | ||
} | ||
} | ||
} |
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.
Sarif is a type of log file for reporting on things like build errors. GitHub's CodeQL analyzer can read these sarif files and incorporate the results into the build process.