The Rubberduck code inspections have just seen yet another structural change, hopefully for the better.
The IInspectionModel interface was originally named IInspection; it only exposes the bare-bones inspection properties, those needed by the CodeInspectionSetting class:
public interface IInspectionModel
{
/// <summary>
/// Gets the inspection type name.
/// </summary>
string Name { get; }
/// <summary>
/// Gets the name of the inspection, without the "Inspection" suffix.
/// </summary>
string AnnotationName { get; }
/// <summary>
/// Gets a short description for the code inspection.
/// </summary>
string Description { get; }
/// <summary>
/// Gets a value indicating the type of the code inspection.
/// </summary>
CodeInspectionType InspectionType { get; }
/// <summary>
/// Gets a value indicating the severity level of the code inspection.
/// </summary>
CodeInspectionSeverity Severity { get; set; }
}
The CodeInspectionSetting type is XML-serialized into the Rubberduck settings file; this allows us to let the user determine an inspection's Severity level:
[XmlType(AnonymousType = true)]
public class CodeInspectionSetting : IInspectionModel
{
[XmlAttribute]
public string Name { get; set; }
[XmlIgnore]
public string Description { get; set; } // not serialized because culture-dependent
[XmlIgnore]
public string AnnotationName { get; set; }
[XmlAttribute]
public CodeInspectionSeverity Severity { get; set; }
[XmlIgnore]
public string SeverityLabel
{
get { return RubberduckUI.ResourceManager.GetString("CodeInspectionSeverity_" + Severity, RubberduckUI.Culture); }
set
{
foreach (var severity in Enum.GetValues(typeof (CodeInspectionSeverity)))
{
if (value == RubberduckUI.ResourceManager.GetString("CodeInspectionSeverity_" + severity, RubberduckUI.Culture))
{
Severity = (CodeInspectionSeverity)severity;
return;
}
}
}
}
[XmlAttribute]
public CodeInspectionType InspectionType { get; set; }
public CodeInspectionSetting()
{
//default constructor required for serialization
}
public CodeInspectionSetting(string name, string description, CodeInspectionType type, CodeInspectionSeverity severity)
{
Name = name;
Description = description;
InspectionType = type;
Severity = severity;
}
public CodeInspectionSetting(IInspectionModel inspection)
: this(inspection.Name, inspection.Description, inspection.InspectionType, inspection.Severity)
{ }
}
The SeverityLabel is used by the settings UI. Does it even belong there?
The IInspection interface exposes additional members, including a method responsible for returning actual inspection results (given a parser state, but that's now an implementation detail):
/// <summary>
/// An interface that abstracts a runnable code inspection.
/// </summary>
public interface IInspection : IInspectionModel, IComparable<IInspection>, IComparable
{
/// <summary>
/// Runs code inspection on specified parse trees.
/// </summary>
/// <returns>Returns inspection results, if any.</returns>
IEnumerable<CodeInspectionResultBase> GetInspectionResults();
/// <summary>
/// Gets a string that contains additional/meta information about an inspection.
/// </summary>
string Meta { get; }
}
Up until two days ago, every code inspection implemented IInspection directly. The recent changes introduce an abstract class, to help remove the redundant code that's common to all implementations:
public abstract class InspectionBase : IInspection
{
protected readonly RubberduckParserState State;
protected InspectionBase(RubberduckParserState state)
{
State = state;
}
public abstract string Description { get; }
public abstract CodeInspectionType InspectionType { get; }
public abstract IEnumerable<CodeInspectionResultBase> GetInspectionResults();
public virtual string Name { get { return GetType().Name; } }
public virtual CodeInspectionSeverity Severity { get; set; }
public virtual string Meta { get { return InspectionsUI.ResourceManager.GetString(Name + "Meta"); } }
// ReSharper disable once UnusedMember.Global: it's referenced in xaml
public virtual string InspectionTypeName { get { return InspectionsUI.ResourceManager.GetString(InspectionType.ToString()); } }
public virtual string AnnotationName { get { return Name.Replace("Inspection", string.Empty); } }
protected virtual IEnumerable<Declaration> Declarations
{
get { return State.AllDeclarations.Where(declaration => !declaration.IsInspectionDisabled(AnnotationName)); }
}
protected virtual IEnumerable<Declaration> UserDeclarations
{
get { return State.AllUserDeclarations.Where(declaration => !declaration.IsInspectionDisabled(AnnotationName)); }
}
public int CompareTo(IInspection other)
{
return string.Compare(InspectionType + Name, other.InspectionType + other.Name, StringComparison.Ordinal);
}
public int CompareTo(object obj)
{
return CompareTo(obj as IInspection);
}
}
The main reason for introducing this base class, was to enable the @Ignore {InspectionName} annotations that the IgnoreOnceQuickFix is inserting, without having to specify in every single inspection that it needs to check for IsInspectionDisabled(AnnotationName). Another reason was to avoid the redundant filtering on !declaration.IsBuiltIn, since most inspections operate on user declarations and their usages.
This is the quick fix class in question:
public class IgnoreOnceQuickFix : CodeInspectionQuickFix
{
private readonly string _annotationText;
private readonly string _inspectionName;
public IgnoreOnceQuickFix(ParserRuleContext context, QualifiedSelection selection, string inspectionName)
: base(context, selection, InspectionsUI.IgnoreOnce)
{
_inspectionName = inspectionName;
_annotationText = "'" + Parsing.Grammar.Annotations.AnnotationMarker +
Parsing.Grammar.Annotations.IgnoreInspection + ' ' + inspectionName;
}
public override bool CanFixInModule { get { return false; } } // not quite "once" if applied to entire module
public override bool CanFixInProject { get { return false; } } // use "disable this inspection" instead of ignoring across the project
public override void Fix()
{
var codeModule = Selection.QualifiedName.Component.CodeModule;
var insertLine = Selection.Selection.StartLine;
var codeLine = insertLine == 1 ? string.Empty : codeModule.get_Lines(insertLine - 1, 1);
var annotationText = _annotationText;
var ignoreAnnotation = "'" + Parsing.Grammar.Annotations.AnnotationMarker + Parsing.Grammar.Annotations.IgnoreInspection;
int commentStart;
if (codeLine.HasComment(out commentStart) && codeLine.Substring(commentStart).StartsWith(ignoreAnnotation))
{
annotationText = codeLine + ' ' + _inspectionName;
codeModule.ReplaceLine(insertLine - 1, annotationText);
}
else
{
codeModule.InsertLines(insertLine, annotationText);
}
}
}
For context, here's an implementation - the ImplicitPublicMemberInspection, which finds public members without an explicit access modifier:
public sealed class ImplicitPublicMemberInspection : InspectionBase
{
public ImplicitPublicMemberInspection(RubberduckParserState state)
: base(state)
{
Severity = CodeInspectionSeverity.Warning;
}
public override string Description { get { return RubberduckUI.ImplicitPublicMember_; } }
public override CodeInspectionType InspectionType { get { return CodeInspectionType.MaintainabilityAndReadabilityIssues; } }
private static readonly DeclarationType[] ProcedureTypes =
{
DeclarationType.Function,
DeclarationType.Procedure,
DeclarationType.PropertyGet,
DeclarationType.PropertyLet,
DeclarationType.PropertySet
};
public override IEnumerable<CodeInspectionResultBase> GetInspectionResults()
{
var issues = from item in UserDeclarations
where ProcedureTypes.Contains(item.DeclarationType)
&& item.Accessibility == Accessibility.Implicit
let context = new QualifiedContext<ParserRuleContext>(item.QualifiedName, item.Context)
select new ImplicitPublicMemberInspectionResult(this, string.Format(Description, ((dynamic)context.Context).ambiguousIdentifier().GetText()), context);
return issues;
}
}
All inspection classes are sealed, and pass the RubberduckParserState dependency down the base type's constructor.
For completeness' sake, here's the accompanying ImplicitPublicMemberInspectionResult class:
public class ImplicitPublicMemberInspectionResult : CodeInspectionResultBase
{
private readonly IEnumerable<CodeInspectionQuickFix> _quickFixes;
public ImplicitPublicMemberInspectionResult(IInspection inspection, string result, QualifiedContext<ParserRuleContext> qualifiedContext)
: base(inspection, result, qualifiedContext.ModuleName, qualifiedContext.Context)
{
_quickFixes = new CodeInspectionQuickFix[]
{
new SpecifyExplicitPublicModifierQuickFix(Context, QualifiedSelection),
new IgnoreOnceQuickFix(qualifiedContext.Context, QualifiedSelection, Inspection.AnnotationName),
};
}
public override IEnumerable<CodeInspectionQuickFix> QuickFixes { get { return _quickFixes; } }
}
All inspection results that can be ignored once have an IgnoreOnceQuickFix; Rubberduck uses this list of quick-fixes to dynamically populate a menu in the code inspection window's toolbar.
Since I introduced the base/abstract class Something doesn't feel right anymore about IInspection: am I right to think it has become superfluous? The base class should simply implement IInspectionModel, and IInspection could be removed - and heck, IInspectionModel could then be renamed back to IInspection as it originally was called. It really feels like CodeInspectionSetting knows more than it needs to about an inspection.
Anything else jumps at you?
