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

HTML templates #299

Merged
merged 221 commits into from
Apr 15, 2021
Merged

HTML templates #299

merged 221 commits into from
Apr 15, 2021

Conversation

tristanlatr
Copy link
Contributor

@tristanlatr tristanlatr commented Nov 29, 2020

Hello,

This PR tries to solve #3, it introduces a new argument: --template-dir.
The users can overwrite any template files contained in the pydoctor/templates.

Custom place holders have been created: header.html, subheader.html and a CSS sheet: extra.css. Overwriting those won't create any warnings on upgrades since they are empty by default. Actual template files have a version number ( independent from pydoctor's ). Overwriting those is supported but might trigger warnings for missing information if the default template version has increase. (So Updating pydoctor doesn't mean all your custom templates will be outdated)

This feature will break (but not crash) the Twisted custom build. Twisted should use the template system to insert custom HTML and JS into the pages.

New errors or warnings:

  • Pydoctor is politely aborting if a template is too new.
  • Pydoctor is warning if a template version cannot be read or the version is outdated.
    With the current implementation it's hard to distinguish a placeholder template (that is not supposed to have a version) and a normal HTML template with invalid version tag...

Structural changes:

  • All pages inherits from Page to avoid duplicating the rendering of : Nav, header.html, footer.html and pageHeader.html.
  • All elements based on a Template file inherits from TemplateElement that is created by passing a ITemplateLoader to the constructor.
  • The TemplateLookup object is core of the templates handling system logic.
  • The Template object represent any file in pydoctor/templates or custom templates files. They are identified by their filenames.
    The Template object is abstract class.
    Template.fromfile is the factory to the concrete templates. For now concrete templates are _HtmlTemplate or _StaticTemplate.
  • The template writer inherits from IWriter that define the 3 methods.

Style changes:

  • add a new CSS class: navlinks: List of links to add in the nav header.
  • Now the project home link is rendering the same way on all pages nicely.
  • I've also fixed a few issue regarding HTML generation: Parameter type information visually hard to find #121 and made the HTML readable on smaller screens.

It load the templates HTML with XMLString. Not sure it's the best practice...

I've also reworded _writeDocsForOne() method so we can understand what's going on

Wrote a draft header file for twisted to try out
Error mesage if the custom template writer class do not support --template-dir
@codecov
Copy link

codecov bot commented Nov 29, 2020

Codecov Report

Merging #299 (e6cd9ac) into master (e32d0ba) will decrease coverage by 0.10%.
The diff coverage is 91.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
- Coverage   87.09%   86.98%   -0.11%     
==========================================
  Files          22       22              
  Lines        4076     4242     +166     
  Branches      821      843      +22     
==========================================
+ Hits         3550     3690     +140     
- Misses        340      360      +20     
- Partials      186      192       +6     
Impacted Files Coverage Δ
pydoctor/epydoc/__init__.py 100.00% <ø> (ø)
pydoctor/driver.py 65.30% <60.60%> (-0.15%) ⬇️
pydoctor/templatewriter/util.py 75.00% <66.66%> (-25.00%) ⬇️
pydoctor/templatewriter/pages/attributechild.py 89.58% <83.33%> (-3.44%) ⬇️
pydoctor/templatewriter/__init__.py 90.37% <90.22%> (-9.63%) ⬇️
pydoctor/epydoc2stan.py 88.52% <93.54%> (+0.28%) ⬆️
pydoctor/templatewriter/pages/__init__.py 84.13% <97.53%> (+2.79%) ⬆️
pydoctor/templatewriter/pages/functionchild.py 89.58% <100.00%> (+0.22%) ⬆️
pydoctor/templatewriter/pages/table.py 95.55% <100.00%> (ø)
pydoctor/templatewriter/summary.py 83.79% <100.00%> (-1.69%) ⬇️
... and 2 more

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 e32d0ba...e6cd9ac. Read the comment docs.

@tristanlatr tristanlatr marked this pull request as draft November 29, 2020 07:33
@mthuurne
Copy link
Contributor

These are just some first impressions, I'll do a more thorough review later.

Thanks for picking this up. I haven't thought about customization in detail yet, but this is certainly a big improvement over monkeypatching.

We should define and document a backwards compatibility strategy. Even if we can't actually solve that problem, having a way to warn "your templates were written for pydoctor oldversion; these things need updating:" would be much better than crashing or silently producing wrong output when using outdated templates.

I think it's best if we merge this after the next release, both because of timing and because of compatibility. I have several ideas on presentation that I want to try out after releasing and some of those might break templates; releasing template customization and then breaking most templates in the release after will not make users happy.

To do so, I used a singleton for the TemplateFileLookup object.

Over the years I've become convinced that singleton is an anti-pattern. It often starts out simple, but it will cause problems later which by then are hard to solve. I should probably do a blog post on it some day. Is it possible to do this without a singleton, via System.options for example?

I've also written a simple header for pydoctor and twisted, as a demo

This is very useful in evaluating the changes. But by the time we're ready to merge, the Twisted templates should be moved to Twisted's repo.

In a quick look over the code, I saw some parts use str for file paths, some use Path and some use FilePath. I think Path offers the nicest interface, so I'd prefer to use that as the primary type for passing file paths.

In situations where we're dealing with paths coming from the source being analyzed and we want to be careful, FilePath might be a better choice. But I don't think that applies to templates, since those are selected by someone who already has enough privileges to construct command lines.

@tristanlatr
Copy link
Contributor Author

I think it's best if we merge this after the next release

I agree

having a way to warn "your templates were written for pydoctor oldversion; these things need updating:" would be much better

Should we store the pydoctor version in every template i.e. <meta name="generator" content="pydoctor 20.12.0" /> and have a script punch it ?

Is it possible to do this without a singleton, via System.options for example?

Probably! But I think this involves using another way than the static method templatefile() to find the templates.
I can write something that will use System.options to create a the System's TemplateFileLookup. But it means that the twisted build have to be changed too.

@tristanlatr
Copy link
Contributor Author

I think Path offers the nicest interface, so I'd prefer to use that as the primary type for passing file paths.

Twisted templates XMLString warns if we pass something other than a FilePath, and it's also the interface used in pydoctor

@mthuurne
Copy link
Contributor

mthuurne commented Dec 3, 2020

Maybe the "generated by" message should be in footer.html, so people can customize that by for example adding their project's logo and navigation links.

@tristanlatr
Copy link
Contributor Author

tristanlatr commented Dec 4, 2020

There two things here:

  • header.html, pageHeader.html and footer.html are place holders for the users to overwrite. They are currently empty by default. This is why users would be able to add custom html and css in those files and upgrade to newer version of pydoctor without risk of breaking stuff.
  • if the users want to customize the actual templates for the twisted templating system, they can. BUT it's where we should be careful about the compatibility of the templates.

I think the meta generator tags should appear in all templates (except the place holders).

Edit: The twisted templates can have missing tags

We don't need to handle them, just print a warning saying the users should check the newer templates and update theirs if they want to.

The module index is only one of the pages this method writes.
By doing the slot filling in code, we ensure it will happen always,
regardless of template customizations that are done.
This removes some inconsistencies between pages, where some were using
slots and others were using renderers (with two different names for
the pydoctor version, too).
@tristanlatr
Copy link
Contributor Author

Hey @mthuurne , do you still want to add more commits to this PR ?

Copy link
Contributor

@mthuurne mthuurne left a comment

Choose a reason for hiding this comment

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

Hey @mthuurne , do you still want to add more commits to this PR ?

Yes, I want to remove as many differences between the different types of pages as possible. I already did a bit for the slots, but there are more slots and renderers that we could make available on all pages. Also using the same footer on every page would be nice (as we discussed in chat).

I have some time available today and tomorrow, so expect new commits soon.

pydoctor/templatewriter/__init__.py Outdated Show resolved Hide resolved
Since node IDs have to be unique within a document, this renderer
could be used only once on each page.

The color set for `#projecthome` was the same as the hover color
that Bootstrap would otherwise set, so removing it doesn't change
anything in the presentation.
There used to be a renderer for a possibly hyperlinked name and a slot
that would insert the name without a link. Now we use a slot at the
page level, to make it available everywhere.

In practice, this means the project name in the footer is now a link.
This was already unused by our templates, but kept around to not break
Twisted. However, Twisted's templates will have to be redesigned
anyway when the template rework lands, so now is a good time to
remove this.

A comment states that the CSS needs to be removed as well, but I can't
find any CSS relevant to this, so I guess it was already removed as
part of earlier cleanups.
@tristanlatr
Copy link
Contributor Author

tristanlatr commented Mar 20, 2021

Hey,

I saw that you removed the special id for the project home button. The fact is that it was not useless. It's effectively not the same color as the bootstrap default: it was slightly blue and would indicate the users that the link would exit the current site. But that's not a big deal.

Also for the footer I'll add some minor CSS changes, you'll tell me what you think

@mthuurne
Copy link
Contributor

I saw that you removed the special id for the project home button. The fact is that it was not useless. It's effectively not the same color as the bootstrap default: it was slightly blue and would indicate the users that the link would exit the current site. But that's not a big deal.

You are correct. It seems I checked the hover color that Bootstrap assigns to the other links in the header, which is equal to the hover color the project link had. But since the project link is inside a <span class="navbar-brand">, the hover color is overridden by Bootstrap to be gray instead of blue.

Shall I override the link hover color for navbar-brand then? Because using an id will create invalid HTML if the project link is on the page more than once, which is now the case with the default template. And even if the default template wouldn't use the link twice, we'd still want to allow user templates to do that, I think.

Also for the footer I'll add some minor CSS changes, you'll tell me what you think

I'll have a look (nothing seems to have been pushed yet).

@tristanlatr
Copy link
Contributor Author

I've done a few minor edits.

For the link color, I've used a css class, which solves the issue while keeping a different hover color for the project home link in the navbar.

I hope that the PR can now be merged!

Co-authored-by: Maarten ter Huurne <maarten@treewalker.org>
@tristanlatr
Copy link
Contributor Author

tristanlatr commented Mar 31, 2021

Hello @mthuurne,

Don't want to be pushy but this PR looks good to me. If you don't like the CSS changes please revert them.
This PR needs to be merged to continue the work on #362 #358 and #370 and #381.

Thanks :-)

The `templatefilepath()` function was only called by pydoctor itself,
so it can be safely removed now.

The `templatefile()` function is not called by Twisted, but is
monkeypatched by Twisted. It does need to exist since Twisted will
save the original function and restore it after it's done. I updated
the docstring to make it clear that Twisted does not call it.
@mthuurne mthuurne merged commit c7d8f9f into twisted:master Apr 15, 2021
@mthuurne
Copy link
Contributor

There are probably still things that can be improved, but it's already a lot better than the old template code and other development work cannot easily use the new template system until it's merged. So I merged it now.

Thanks for your hard work and your patience.

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.

None yet

2 participants