Skip to content
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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,6 @@ DatabaseTemplate/*

# .Net Tools
tools/*
Ed-Fi-Db-Deploy.log
EdFi\.CodeGen\.log
downloads/
reports/

Expand All @@ -123,3 +121,7 @@ environment.json

# Sandbox Admin
/**/SandboxAdminLog*

# Log output
*.sarif
Copy link
Contributor Author

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.

*.log
140 changes: 140 additions & 0 deletions Application/.editorconfig
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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -5,7 +5,6 @@

using System.Data.Entity;
using EdFi.Common.Configuration;
using EdFi.Ods.Common.Configuration;
using Npgsql;

namespace EdFi.Ods.Sandbox.Admin.Contexts
Expand All @@ -32,4 +31,4 @@ public DatabaseEngineDbConfiguration(DatabaseEngine databaseEngine)
SetDefaultConnectionFactory(connectionFactory: new NpgsqlConnectionFactory());
}
}
}
}
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.
Expand Down
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.
Expand Down Expand Up @@ -55,4 +55,4 @@ protected override void ApplyProviderSpecificConnection(DbContextOptionsBuilder
optionsBuilder.UseNpgsql(_connectionString);
}
}
}
}
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.
Expand All @@ -21,4 +21,4 @@ protected override void ApplyProviderSpecificConnection(DbContextOptionsBuilder
optionsBuilder.UseSqlServer(_connectionString);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public async Task<ActionResult> Create(CreateLoginModel model)
}

// Failed Model Validation
Response.StatusCode = (int)HttpStatusCode.BadRequest;
Response.StatusCode = (int) HttpStatusCode.BadRequest;
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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());
}

Expand Down Expand Up @@ -245,7 +245,7 @@ public ActionResult PasswordResetSuccessful()
return View(
new PasswordResetCompletViewModel
{
Message = (string)message
Message = (string) message
});
}

Expand Down Expand Up @@ -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
Expand All @@ -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());
}

Expand Down Expand Up @@ -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.
Expand All @@ -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
{
Expand Down Expand Up @@ -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}");
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Append instead of AppendLine - but if this unhandled exception ever occurs, it will be easier to interpret the concatenated messages if there are linebreaks.

counter++;
currentException = currentException.InnerException;
}
Expand Down Expand Up @@ -170,7 +171,7 @@ public async Task<IActionResult> PostClient([FromBody] SandboxClientCreateModel
}
}

return StatusCode((int)HttpStatusCode.NotAcceptable);
return StatusCode((int) HttpStatusCode.NotAcceptable);
}

[HttpPut("{id}")]
Expand Down Expand Up @@ -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.
Expand All @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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; }
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
//}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
{
Expand All @@ -70,4 +52,4 @@ public void Delete(int id)
_clientAppRepo.DeleteUser(user);
}
}
}
}
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.
Expand All @@ -15,4 +15,4 @@ public ActionResult Index()
return View();
}
}
}
}
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.
Expand Down
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.
Expand Down Expand Up @@ -111,4 +111,4 @@ public ActionResult RemoveOrphans()
});
}
}
}
}
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.
Expand All @@ -15,4 +15,4 @@ public ActionResult Initialize()
return Content("Successfully initialized test users.");
}
}
}
}
Loading