Skip to content

Conversation

@melgoharyme
Copy link

Description

This PR improves the SafeAreaEdgesTypeDesignConverter by clarifying the validation logic for easier maintenance.

  • Simplified loops for checking valid SafeAreaRegions values.
  • Kept essential comments for better readability.
  • No changes to existing behavior; all validation rules remain unchanged.

Issues Fixed

No functional bugs were fixed; this PR focuses on code cleanup and readability.

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Jan 2, 2026
@dotnet-policy-service
Copy link
Contributor

Hey there @@melgoharyme! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

🔴 Remove StringExtensions Public Class

This is a design-time assembly consumed by Visual Studio. Adding a public StringExtensions class:

  • Pollutes the API surface with generic utilities
  • Uses a very common name that could conflict with user code
  • Duplicates BCL functionality (string.IsNullOrEmpty, string.Equals)

🟡 Use Established Pattern

Looking at VisibilityDesignTypeConverter.cs in the same project, there's an established pattern using HashSet<string> with StringComparer.OrdinalIgnoreCase:

public class SafeAreaEdgesTypeDesignConverter : StringConverter
{
    private static readonly HashSet<string> ValidValues = new HashSet<string>(
        new[] { "All", "None", "Default", "SoftInput", "Container" },
        StringComparer.OrdinalIgnoreCase);

    public override bool IsValid(ITypeDescriptorContext context, object value)
    {
        // MUST MATCH SafeAreaEdgesTypeConverter.ConvertFrom
        if (value?.ToString()?.Trim() is not string strValue || strValue.Length == 0)
            return false;

        var parts = strValue.Split(',');

        // Must have 1, 2, or 4 parts
        if (parts.Length != 1 && parts.Length != 2 && parts.Length != 4)
            return false;

        // Each part must be a valid SafeAreaRegions value
        foreach (var part in parts)
        {
            if (!ValidValues.Contains(part.Trim()))
                return false;
        }

        return true;
    }
}

@melgoharyme melgoharyme requested a review from PureWeen January 6, 2026 07:15
@melgoharyme
Copy link
Author

@PureWeen I've addressed your feedback and pushed the changes. Could you please take another look when you have a moment? Thanks!

@melgoharyme melgoharyme deleted the refactor/safeareaedges-converter branch January 22, 2026 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community ✨ Community Contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants