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

Wrap signatures onto several lines when function len is over a treshold and function has the focus #831

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

Conversation

tristanlatr
Copy link
Contributor

@tristanlatr tristanlatr commented Oct 25, 2024

This change fixes #801 as well as introduce a rather a lot of refactoring and new ParsedDocstring features.

  • Parameters html are divided into .sig-param or .sig-symbol spans.
  • When the function is long enough (88 chars excluding the def and ending colon, this is hard coded) an extra CSS class .long-signature is added to the parent function-signature.
  • The first parameter 'cls' or 'self' of (class) methods is marked with the 'undocumented' CSS class, this way it's clearly not part of the API.
  • Add some CSS to expand the signature of long functions when they have the focus.
  • Refactor CSS rules for parameters tables to be more simpler and predictable with percentage values for smaller screens.
  • Remove ValueFormatter classes since they were only a hack to show HTML tags inside Signature.__str__(), which we don't need anymore because we're computing the representation ourselve.
  • Remove ParsedStanOnly class because we know have better alternatives.
  • Introduce ParsedDocstring.to_text() -> str.
  • ParsedDocstring.with_tag(Tag) -> ParsedDocstring.
  • ParsedDocstring.combine(list[ParsedDocsrting]) -> ParsedDocstring.
  • When a function has invalid parameters its rendered as (…) instead of confusing () - like it was a zero argument callable.

Examples:

  • Not wrapped because overloaded function never get wrapped.

Capture d’écran, le 2024-11-15 à 11 48 35

  • Wrapped because the function length is over 88 chars

Capture d’écran, le 2024-11-15 à 11 49 30

The way to get the old behaviour back is to use the following custom css:

.sig-param, .sig-symbol {
    display: initial !important;
    margin-left: 0 !important;
    padding-left: 0 !important;
}

…ill help to construct one parsed docstring from several parts.
We mimic the Signature.__str__ method for the implementation but instead of returning a str we return a ParsedDocstring, which is far more convenient.

This change fixes #801:
- Parameters html are divided into .sig-param spans.
- When the function is long enought an extra CSS class .expand-signature is added to the parent function-signature.
- The first parameter 'cls' or 'self' of (class) methods is marked with the 'undocumented' CSS class, this way it's clearly not part of the API.
- Add some CSS  to expand the signature of long functions when they have the focus only.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 96.25000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 92.85%. Comparing base (02063d3) to head (56168f8).

Files with missing lines Patch % Lines
pydoctor/epydoc/markup/__init__.py 90.90% 6 Missing ⚠️
pydoctor/epydoc2stan.py 97.11% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #831      +/-   ##
==========================================
+ Coverage   92.79%   92.85%   +0.05%     
==========================================
  Files          47       47              
  Lines        8468     8588     +120     
  Branches     1550     1578      +28     
==========================================
+ Hits         7858     7974     +116     
- Misses        350      353       +3     
- Partials      260      261       +1     

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

This comment has been minimized.

… docstrings they only update the local to_stan() method dynamically.
…w parsed docstrings they only update the local to_stan() method dynamically."

This reverts commit eca5ced.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

2 similar comments

This comment has been minimized.

This comment has been minimized.

…s when overloads are overwhelming... This probably requires some more tweaks but it's still better than showing everything at once.

This comment has been minimized.

This comment has been minimized.

Fix some cyclic imports issue as a drive-by change: model.Documentable was uncessarly runtime imported inside restructuredtext and epydoc parsers.

This comment has been minimized.

This comment has been minimized.

1 similar comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

2 similar comments

This comment has been minimized.

This comment has been minimized.

README.rst Show resolved Hide resolved
Copy link

According to pydoctor_primer, this change doesn't affect pydoctor warnings on a corpus of open source code. ✅

@@ -997,7 +976,7 @@ def colorized_pyval_fallback(_: List[ParseError], doc:ParsedDocstring, __:model.
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this docstring is wrong


if param.default is not _empty:
if param.annotation is not _empty:
# TODO: should we keep these two different manners ?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link

According to pydoctor_primer, this change doesn't affect pydoctor warnings on a corpus of open source code. ✅

@tristanlatr tristanlatr requested review from a team January 14, 2025 18:32
@tristanlatr tristanlatr changed the title Wrap signatures onto several lines when function len is over a treshold Wrap signatures onto several lines when function len is over a treshold and function has the focus Jan 15, 2025
@tristanlatr
Copy link
Contributor Author

This would need a review. I don’t know how to get this done other than asking you directly @glyph. It’s been a while that I’m waiting…

@glyph
Copy link
Member

glyph commented Jan 22, 2025

@tristanlatr sorry for the delay, it'll probably be a few more days (pydoctor reviews are extra hard, in addition to having limited time!) but these are on my to-do list.

@tristanlatr
Copy link
Contributor Author

Thanks for your message @glyph, please take your time. It's good to see that it's in the todo list of someone somewhere already. I'm sorry if this PR comes with rather a few refactors... I could try to split it up... Tell me if that would help you. Thanks

@tristanlatr tristanlatr added the pending This ticket needs code related to another ticket not yet fixed label Jan 31, 2025
Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

I do wish I understood this code a bit better, but the type checker is doing some good heavy lifting assuaging my concerns :). The new style looks great, and I look forward to deploying this. Thanks!

@@ -297,4 +297,4 @@ class ExamplePEP526Class:
"""

attr1: str
attr2: int
attr2: int
Copy link
Member

Choose a reason for hiding this comment

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

Is this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don’t think so…


@property
def has_body(self) -> bool:
return any(e.has_body for e in self._elements)
Copy link
Member

Choose a reason for hiding this comment

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

Huh, why the coverage gap on these properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because has_body is only used at one place: in the properties documentation processing. And properties don’t have this kind of parsed docstring. But I can always add a small test to cover that branch.

Comment on lines +201 to +203
def colorize_pyval(pyval: Any, linelen:Optional[int], maxlines:int,
linebreakok:bool=True, refmap:Optional[Dict[str, str]]=None,
is_annotation: bool = False) -> ColorizedPyvalRepr:
Copy link
Member

Choose a reason for hiding this comment

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

should we be adopting Black so that this sort of style change doesn't require discretion? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we should… #851 and PR #859. But I don’t have the courage to deal with the merge conflicts yet

"""
#TODO: Fix this soon
Copy link
Member

Choose a reason for hiding this comment

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

Link to an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. #873.
And even a PR that is a work in progress #874

# some ParsedDocstring subclass raises NotImplementedError on calling to_node()
# Like ParsedPlaintextDocstring.
doc = source.docstring
doc = ob.parsed_docstring.to_text()
Copy link
Member

Choose a reason for hiding this comment

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

lovely to see an ugly too-broad try like this go away :)

pydoctor/test/epydoc/test_pyval_repr.py Outdated Show resolved Hide resolved
Co-authored-by: Glyph <[email protected]>
Copy link

github-actions bot commented Feb 4, 2025

According to pydoctor_primer, this change doesn't affect pydoctor warnings on a corpus of open source code. ✅

Copy link
Contributor Author

@tristanlatr tristanlatr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your review @glyph. I’ll adjust a few things as per your comment and merge that. I’de just like to have merged #872 before so the parameter style related changes can be removed from this PR

@@ -253,8 +253,15 @@ ul ul ul ul ul ul ul {

/* Argument name + type column table */
.fieldTable tr td.fieldArgContainer {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
.fieldTable tr td.fieldArgContainer {
.fieldTable tr td.fieldArgContainer {
width: 325px;

Comment on lines +257 to +264
/* It not seems to work with percentage values
so I used px values, these are just indications for the
CSS auto layout table algo not to create tables
with rather unbalanced columns width. */
max-width: 400px;
}
.fieldTable tr td.fieldArgDesc {
max-width: 600px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
/* It not seems to work with percentage values
so I used px values, these are just indications for the
CSS auto layout table algo not to create tables
with rather unbalanced columns width. */
max-width: 400px;
}
.fieldTable tr td.fieldArgDesc {
max-width: 600px;

Comment on lines +315 to +321
.fieldTable{
table-layout: fixed;
}
/* Argument name + type column table */
.fieldTable tr td.fieldArgContainer {
width: 34%;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
.fieldTable{
table-layout: fixed;
}
/* Argument name + type column table */
.fieldTable tr td.fieldArgContainer {
width: 34%;
}

#childList a:target ~ .functionBody{
background-color: transparent;
box-shadow: none;
box-shadow: 7px 1px 0px 3px rgb(253 255 223), -36px 1px 0px 3px rgb(253 255 223);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
box-shadow: 7px 1px 0px 3px rgb(253 255 223), -36px 1px 0px 3px rgb(253 255 223);
background-color: transparent;
box-shadow: none;

@@ -320,24 +333,13 @@ ul ul ul ul ul ul ul {
#splitTables > table tr td:nth-child(2) {
width: 160px;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
}
}
/* Argument name + type column table */
.fieldTable tr td.fieldArgContainer {
width: 170px;
}
.fieldTable {
table-layout: fixed;
}

@@ -328,6 +420,20 @@ def switch_context(self, ob:Optional['Documentable']) -> ContextManager[None]:
in this case error will NOT be reported at all.
"""

class NotFoundLinker(DocstringLinker):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The NotFoundLinker is already present in linker.py no need to have it here

@@ -297,4 +297,4 @@ class ExamplePEP526Class:
"""

attr1: str
attr2: int
attr2: int
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don’t think so…

"""
#TODO: Fix this soon
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. #873.
And even a PR that is a work in progress #874

Comment on lines +201 to +203
def colorize_pyval(pyval: Any, linelen:Optional[int], maxlines:int,
linebreakok:bool=True, refmap:Optional[Dict[str, str]]=None,
is_annotation: bool = False) -> ColorizedPyvalRepr:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we should… #851 and PR #859. But I don’t have the courage to deal with the merge conflicts yet


@property
def has_body(self) -> bool:
return any(e.has_body for e in self._elements)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because has_body is only used at one place: in the properties documentation processing. And properties don’t have this kind of parsed docstring. But I can always add a small test to cover that branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending This ticket needs code related to another ticket not yet fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTML: Break function signature elements into multiple spans
2 participants