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

Add additional XmlQualifiedName for guid #403

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

Conversation

bzwieratinnovadis
Copy link

@mganss Just to make your life easier I created a PR

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (8c58cf2) 94.00% compared to head (44ee716) 94.01%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #403   +/-   ##
=======================================
  Coverage   94.00%   94.01%           
=======================================
  Files          21       21           
  Lines        3021     3025    +4     
  Branches      479      479           
=======================================
+ Hits         2840     2844    +4     
  Misses        119      119           
  Partials       62       62           
Files Changed Coverage Δ
XmlSchemaClassGenerator/CodeUtilities.cs 92.63% <100.00%> (+0.10%) ⬆️

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

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

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

Here's a review and some improvements for your code:

Review of Changes:

  1. Variable Name Change: Changed GuidQualifiedName to GuidQualifiedNames as it now holds an array of XmlQualifiedName.
  2. Array Initialization: Initialized GuidQualifiedNames with two instances of XmlQualifiedName.
  3. SchemaType Check Improvement: Changed schemaType.IsDerivedFrom(GuidQualifiedName) to GuidQualifiedNames.Any(schemaType.IsDerivedFrom) for checking if schemaType is derived from any of the GuidQualifiedNames.

Improvements:

  1. Consistency in Naming: Ensure that variable names (GuidQualifiedNames) accurately reflect their purpose and are consistently used throughout the codebase.

  2. Clarity and Readability: Consider using more descriptive variable names where appropriate to improve code readability.

  3. Error Handling: Implement error handling or logging mechanisms to manage scenarios where schemaType does not match any of the expected GuidQualifiedNames.

  4. Unit Testing: Write unit tests to validate the behavior of the GetEffectiveType method under various scenarios, including different schemaType inputs.

Consolidated Code (with improvements):

Type FromFallback() => configuration.UseIntegerDataTypeAsFallback && configuration.IntegerDataType != null ? configuration.IntegerDataType : typeof(string);

private static readonly XmlQualifiedName[] GuidQualifiedNames =
{
    new XmlQualifiedName("guid", "http://microsoft.com/wsdl/types/"),
    new XmlQualifiedName("guid", "http://schemas.microsoft.com/2003/10/Serialization/")
};

public static Type GetEffectiveType(this XmlSchemaDatatype type, GeneratorConfiguration configuration, IEnumerable<RestrictionModel> restrictions, XmlSchemaType schemaType, bool attribute = false)
{
    Type resultType = attribute ? typeof(string) : type.ValueType;

    if (GuidQualifiedNames.Any(schemaType.IsDerivedFrom))
    {
        resultType = typeof(Guid);
    }

    return resultType;
}

Summary:

  • Naming and Readability: Improved by using descriptive names (GuidQualifiedNames) and ensuring consistency.
  • Functionality: Enhanced by using array initialization for GuidQualifiedNames and checking against any of these names in GetEffectiveType.
  • Maintainability: Improved through clearer logic and potential error handling suggestions.

Feel free to integrate these suggestions into your codebase/

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.

2 participants