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 option --privacy #517

Merged
merged 28 commits into from
Mar 14, 2022
Merged

Conversation

tristanlatr
Copy link
Contributor

@tristanlatr tristanlatr commented Mar 14, 2022

Add option --privacy to set the privacy of specific objects when default rules doesn't fit the use case.

Finally fix #36 and #461.

Tackles part of #315.

The help:

--privacy=<PRIVACY>:<PATTERN>
                        Set the privacy of specific objects when default rules
                        doesn't fit the use case. Format:
                        '<PRIVACY>:<PATTERN>', where <PRIVACY> can be one of
                        'PUBLIC', 'PRIVATE' or 'HIDDEN' (case insensitive),
                        and <PATTERN> is fnmatch-like pattern matching objects
                        fullName. Pattern added last have priority over a
                        pattern added before, but an exact match wins over a
                        fnmatch. Can be repeated.

The docs: https://pydoctor--517.org.readthedocs.build/en/517/customize.html#override-objects-privacy-show-hide

@tristanlatr tristanlatr requested a review from adiroiban March 14, 2022 00:57
@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #517 (5f6b53c) into master (f8bd799) will increase coverage by 0.05%.
The diff coverage is 95.23%.

❗ Current head 5f6b53c differs from pull request most recent head 8a8f9ad. Consider uploading reports for the commit 8a8f9ad to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
+ Coverage   90.16%   90.21%   +0.05%     
==========================================
  Files          35       36       +1     
  Lines        6507     6584      +77     
  Branches     1464     1484      +20     
==========================================
+ Hits         5867     5940      +73     
- Misses        389      392       +3     
- Partials      251      252       +1     
Impacted Files Coverage Δ
pydoctor/driver.py 66.13% <66.66%> (-0.26%) ⬇️
pydoctor/model.py 93.54% <100.00%> (+0.26%) ⬆️
pydoctor/qnmatch.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8bd799...8a8f9ad. Read the comment docs.

@tristanlatr
Copy link
Contributor Author

So a new import in test module made introspection test crash because of recursion error (while introspecting typing.List) :/
I don't think we should maintain the introspection code, but rather rely on something else. Astroid can build AST trees from live modules, so once we enter the new generation of pydoctor with astroid, we won't have to maintain our introspection code anymore.

So to fix this I'll just do the easiest hack.

@tristanlatr
Copy link
Contributor Author

View example here: https://pydoctor--517.org.readthedocs.build/en/517/api/

The last readthedocs build failed because "Concurrency limit reached (2)", so that's not related with the changes.

@tristanlatr tristanlatr linked an issue Mar 14, 2022 that may be closed by this pull request
@tristanlatr
Copy link
Contributor Author

tristanlatr commented Mar 14, 2022

I feel like in order to be really helpful we should use regexes instead of fnmatch, because there are cases where it could be difficult to work with. Like if we'd like to make all top-level names in a module public, but not stuff under the top level. This would be highly impractical with the current implementation. But maybe it's good enough anyway. It satisfies the Twisted use case anyway.

A new options could be added later if we feel positive about this, like --privacy-re.

What do you think. @adiroiban ?

Edit: This could also be fixed by having * and ** , the later would mean that the wildcard should recurse into objects members (Like how glob does).

@adiroiban
Copy link
Member

I don't know what to say about glob vs regex.

At work for an end-user product I have implemented a custom enhanced glob.

I like regex, but for end users I feel that regex is on the other extreme - super powerful and complex.

So, maybe just go with glob and later add some sort of git pattern matching, and further improvements.

And I would say that --privacy-re should only be implemented is someone asked for it.. and sends a PR :)

At work, for a progressive regex support I did something like this:
You have the s/REGEX/FLAGS format. It's not idea, but in practice I never
had a customer that had a glob expression starting with regex/ and ending with something that look like valid regex flags

--privacy="public:regex/twisted.test.proto_helpers.*/i"

So I would say , keep it simple :)


Not sure if it should be called HIDDEN or IGNORED.

I think that the documentation should be extend to document the "standard" privacy types/classes

  • Public - Always and visible rendered in HTML
  • Private - Rendered in HTML, but hidden via CSS by default
  • hidden - No HTML markup is generated and no links are created.

Not sure if --privacy= is the best name for this functionality.

Maybe it can be implemented as a --tag that later can be used in a theme to implement custom behaviour.


What I can image, is building the apidocs in 2 versions:

  • One version with only public API - this used by the external users / customers
  • One version with private API hidden - this is used by the internal development teams

I think that this is on the right direction.

But before doing a full review, it would help to have the documentation, to make sure I understand the scope and target of these changes.


I guess that we have 2 main use cases, and I think that these 2 examples can be part of the docs:

  • Make everything public
  • Keep normal convention for public and private api, but ignore/exclude/hide the test code

Thanks!

@tristanlatr
Copy link
Contributor Author

Not sure if --privacy= is the best name for this functionality. Maybe it can be implemented as a --tag that later can be used in a theme to implement custom behaviour.

I think what your proposing to much more complicated and users might just don't understand the --tag argument. --privacy makes it obvious we're talking about privacy teaks. I also thought of something like --override-privacy or --force-priv.

What I can image, is building the apidocs in 2 versions: One version with only public API - this used by the external users / customers One version with private API hidden - this is used by the internal development teams

Here again, I think this kind of feature will male pydoctor more complicated to work with. I feel like #492 is a better fix for the same issue.

But before doing a full review, it would help to have the documentation, to make sure I understand the scope and target of these changes.

I will add more docs..

This could also be fixed by having * and ** , the later would mean that the wildcard should recurse into objects members (Like how glob does).

I've implemented and tested this, it's really not very complicated, so I'de suggest we adopt it.

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

I've implemented and tested this, it's really not very complicated, so I'de suggest we adopt it.

Agree.

Also, happy to keep it like --privacy . No need for tagging.

Looking forward to do a big purge in twisted/twisted and just use standard pydoctor :)

Thanks!

@tristanlatr tristanlatr requested a review from adiroiban March 14, 2022 21:01
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Excellent work. Only minor comments.

Thanks!

@tristanlatr
Copy link
Contributor Author

I’ve added a custom match function and rather complete documentation. Tell me what you think.

happy to keep it like --privacy . No need for tagging.

Not even sure what you are referring as tagging. The theming system is really only about templates files. Themes cannot carry custom code at this time, and probably never.

But I agree the option name could be more explicit.

@tristanlatr
Copy link
Contributor Author

Merging this, thanks @adiroiban for your reviews

@tristanlatr tristanlatr merged commit 63b6f3d into twisted:master Mar 14, 2022
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.

Command line option to show all private objects pydoctor should be able to ignore 'test' directories
2 participants