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

SA1642 (constructor summary text) special case for incorrect class name #415

Closed
sharwell opened this issue Jan 24, 2015 · 9 comments · Fixed by #1517
Closed

SA1642 (constructor summary text) special case for incorrect class name #415

sharwell opened this issue Jan 24, 2015 · 9 comments · Fixed by #1517
Assignees
Milestone

Comments

@sharwell
Copy link
Member

When SA1642 is reported, it is always reported for the entire <summary> element. In each of the following scenarios, a more specific location should be used for reporting the warning.

  1. When the name of the class is not wrapped in a <see> element, but the text is otherwise correct. For example, the warning reported for the following code should only underline the word Customer:

    /// <summary>
    /// Initializes a new instance of the Customer class.
    /// </summary>
    public Customer() { }
  2. As in the first case, but now considering an incorrect class name. For example:

    /// <summary>
    /// Initializes a new instance of the NotTheRightType class.
    /// </summary>
    public Customer() { }
  3. When the name of the class specified in the cref attribute does not match the name of the current class, but the text is otherwise correct. In this case, the warning should be reported for the cref value instead of the entire <summary> element. For example:

    /// <summary>
    /// Initializes a new instance of the <see cref="NotTheRightType"/> class.
    /// </summary>
    public Customer() { }
@sharwell sharwell added the bug label Jan 24, 2015
@sharwell sharwell added this to the 1.0.0 Alpha 2 milestone Jan 24, 2015
@sharwell
Copy link
Member Author

I initially assigned @pdelvo since he did a great job with the initial implementation. I'll take a look soon if you don't have time.

@sharwell
Copy link
Member Author

💡 The code fix for each of the above cases should replace the current type reference with the corrected type reference, as opposed to inserting the entire standard prefix.

@sharwell
Copy link
Member Author

❓ I also noticed a case like the following:

Initializes a new instance of the <see cref="MyType"/> with A and B.

Notice that everything is correct except the word "class" has been omitted. Should we special case handling for code that is close but not exactly correct? If so, how close is close?

@pdelvo
Copy link
Member

pdelvo commented Jan 26, 2015

This is getting complicated. Calculating the Levenshtein distance and having an upper limit would make this rule almost useless. If I have a spelling mistake in there the distance would be very small and I would not get a diagnostic. But we could add a few special cases like the one you mentioned.

@sharwell
Copy link
Member Author

I tend to agree but wanted to ask the question publicly before just deciding 👍

@sharwell
Copy link
Member Author

Doh. I thought I made a new issue for the "closeness" question.

@sharwell sharwell reopened this Jan 26, 2015
@sharwell
Copy link
Member Author

The incorrect class name situation is common and relatively simple to detect/correct.

@pdelvo
Copy link
Member

pdelvo commented Jan 29, 2015

I am working on this. It is getting a bit complicated but my solution is able to also detect spelling mistakes and changing the comment instead of adding a new one

@sharwell
Copy link
Member Author

Well that would be cool 👍 It's OK if it doesn't work out too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants