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

Feature/exclude private #10

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

tmonck
Copy link

@tmonck tmonck commented Jan 23, 2022

Should resolve #3. I am open to suggestions if you want this to look a little differently or prefer different names.

@tmonck tmonck requested a review from p1va as a code owner January 23, 2022 18:24
@tmonck
Copy link
Author

tmonck commented Feb 5, 2022

Just following up. If this should be a discussion prior to a PR let me know and I'll decline this PR and take the suggestion back to the current issue.

@p1va
Copy link
Owner

p1va commented Feb 6, 2022

Apologies for the late reply and thank you 🙏 for taking the time to look into this as it's by far the most requested feature.
I heard many times people saying "I would like to document just publicly visible members" so i was wondering if we could design this in that way where we specify a list of access modifiers to consider rather than exclude.

access_modifiers:
  - public
  - internal

It is also worth knowing that things like static or async are considered modifiers as well as per this code (they end up in node.Modifiers) but probably this filtering capability should not be applied to them and just restricted to access ones (public,
protected internal, protected, internal, private, protected private).

Dry Run

The logic that detects which members need to be documented need to be refactored to avoid breaking the dry run functionality (e.g. returning non 0 exit code on private member without doc when configured that they shouldn't have any).

This is because it assumes that if a member of a SyntaxKind doesn't have documentation it has to be applied to it 100% of the times but after this change the knowledge of whether or not a member should be documented should live in Strategies rather than in the DocumentSyntaxWalker.
I ll think about it and get back to you on this point.
As of now i am thinking something like refactoring Strategies to return OneOf<MethodDeclarationSyntax, MemberUnchanged> could help here.

@tmonck
Copy link
Author

tmonck commented Feb 6, 2022

I agree with the pattern of people specifying what they want to include instead of what they want to exclude, I will update the code for that pattern.

Since modifiers is an Array we will need to make sure if item in the list is something like protected internal that we ensure both are found in the Array. I do believe the Modifiers have an extension method of ToFullString() which might allow us to compare the item in the include list to the full access modifier. I will look at some refactoring while you are thinking about the approach that's best for the project

@tmonck
Copy link
Author

tmonck commented Feb 13, 2022

@p1va I've pushed an update. I'm not sure if this is what you were looking for but it allows the strategy to determine if the node should be documented. It also works correctly for dry run such that if the node shouldn't be documented it's not printed out. Let me know your thoughts when you get some time.

If you like it I still need to update the README to document the way you adjust what gets documented.

@infor-ajdragasa
Copy link

This feature was never merged?

image


foreach (var modifier in options.AccessModifiers)
{
var modifierArray = modifier.Split(" ");
Copy link

Choose a reason for hiding this comment

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

@tmonck I cloned your repo to try this out and this logic doesn't seem to work. For example, when I just had "public" in the access modifiers list in the config, the if (modifierArray.Length > 1) statement gets skipped over entirely

Copy link

@gmkado gmkado Apr 24, 2024

Choose a reason for hiding this comment

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

This is my workaround

private static readonly List<string> _accessorKeyWords =
[
    "public",
    "internal",
    "private",
    "protected"
];

public static bool ShouldDocument(this MemberDeclarationSyntax node, MemberDocumentationOptionsBase options)
{
    var nodeModifiers = node.Modifiers.Select(m => m.Text)
        .Where(mText=> _accessorKeyWords.Contains(mText));
    return options.AccessModifiers.Any(m=>
    {
        var optionModifier = m.Split(' ');
        return nodeModifiers.All(nodeModifier => optionModifier.Contains(nodeModifier));
    });
}

This doesn't allow the user to specify other valid modifiers besides what's in _accessorKeyWords (e.g. readonly) so not sure if this works for your implementation

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.

Add option to not add documentation for private methods/classes
4 participants