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

Revise rules and being to add meta data #200

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

joeldickson
Copy link
Contributor

@joeldickson joeldickson commented Nov 18, 2024

There 2 changes in this PR.

  1. Update docs
  2. Add metadata about tech debt time to each rule

The second part is about me wanting to collect some more accurate data about how much tech debt is in a system, the reason i want to use the Analyzers for it is i want to be able to enforces as a part of linting creation people to think about how much work it is to fix the things they are flagging, because the person creating the rule will be the best one to make this call.

At the moment its just a simple "Time in minutes" we want to capture so that when we analyze a code base we can get a weighted estimate of how much technical debt for which problems are in there.

This is similar to the approach in software like Sonarqube

Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 93.39623% with 14 lines in your changes missing coverage. Please review.

Project coverage is 75.81%. Comparing base (5f5ecf7) to head (3b73b74).

Files with missing lines Patch % Lines
...ers/AgodaCustom/AG0011NoDirectQueryStringAccess.cs 71.42% 4 Missing ⚠️
...odaCustom/AG0001DependencyResolverMustNotBeUsed.cs 33.33% 2 Missing ⚠️
...odaCustom/AG0032PreventUseOfBlockingTaskMethods.cs 33.33% 2 Missing ⚠️
...lyzers/AgodaCustom/AG0033PreventUseOfTaskResult.cs 33.33% 2 Missing ⚠️
...yzers/AgodaCustom/AG0035PreventUseOfMachineName.cs 33.33% 2 Missing ⚠️
...om/AG0040WaitUntilStateNetworkIdleMustNotBeUsed.cs 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   75.21%   75.81%   +0.60%     
==========================================
  Files          70       70              
  Lines        2606     2729     +123     
  Branches      317      317              
==========================================
+ Hits         1960     2069     +109     
- Misses        564      578      +14     
  Partials       82       82              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


Each component registered with the dependency injection container should have exactly one public constructor. Having multiple public constructors leads to ambiguity in dependency resolution and makes the codebase harder to maintain.

This is a blocker rule - any component with multiple public constructors should be refactored to have only one.
Copy link
Member

Choose a reason for hiding this comment

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

What does this callout for being "a blocker rule" mean, and is it important? Is it related to severity somehow? Ppl can override it anyway, no?

doc/AG0012.md Show resolved Hide resolved
- Refactoring the code to better expose the behavior through public methods
- Rethinking the design to avoid the need to test internals

### Don't ❌
Copy link
Member

Choose a reason for hiding this comment

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

Can we unify? Either use the cute icons, or not - for every doc (13 and before don't seem to have it)?


private static Dictionary<string, string> _props = new Dictionary<string, string>()
{
{ AnalyzerConstants.KEY_TECH_DEBT_IN_MINUTES, "10" }
Copy link
Member

Choose a reason for hiding this comment

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

I think some of these estimates are very optimistic :)

@@ -1,4 +1,5 @@
using System.Collections.Immutable;
using System.Collections.Generic;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to bring in new using but there seems to be no new code added that would require it

@@ -1,4 +1,5 @@
using System.Collections.Immutable;
using System.Collections.Generic;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to bring in new using but there seems to be no new code added that would require it

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.

4 participants