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

Adapt docgen to use the mkdocs material theme #4226

Merged
merged 12 commits into from
Nov 8, 2022
Merged

Conversation

mfelsche
Copy link
Contributor

@mfelsche mfelsche commented Nov 3, 2022

As the ponylang-mkdocs-theme wasn't maintainable anymore. This puts all the necessary tweaks inside the generated files (logo, custom css, theme adaptions and extensions) and only depends on the user to have mkdocs and the python package material-mkdocs installed to generate and serve html docs.

@mfelsche mfelsche added changelog - changed Automatically add "Changed" CHANGELOG entry on merge documentation Improvements or additions to documentation discuss during sync Should be discussed during an upcoming sync labels Nov 3, 2022
@ponylang-main
Copy link
Contributor

Hi @mfelsche,

The changelog - changed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4226.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@mfelsche
Copy link
Contributor Author

mfelsche commented Nov 4, 2022

Windows build is failing.

It seems we either compile stuff on windows as C++ or not with C99 standard. Investigating... Any help appreciated.

@SeanTAllen
Copy link
Member

SeanTAllen commented Nov 4, 2022

This also contains an improvement to sort items in a package alphabetically to make it easier to find classes/actors/primitives.

I'm not sure that is an improvement. Can you revert that change and we can discuss it as a separate change?

@SeanTAllen
Copy link
Member

@mfelsche I dont see the Window build as having failed. I see an arm build failing. Can you clarify?

@chalcolith
Copy link
Member

I am in favour of alphabetizing. Hunting for things in the docs is a perennial source of friction for me.

@SeanTAllen
Copy link
Member

I assume that people put thought into the order of their methods and randomly moving them around based on name is "not a particularly good thing" when listing in docs. As I said, I'd like to revert the change as it isn't specifically part of this change and it can be discussed further elsewhere.

@mfelsche
Copy link
Contributor Author

mfelsche commented Nov 4, 2022

Fair point about the ordering being unrelated here. Will put it into another PR. One additional point: Only the order of classes/primitives/actors is affected, not the order of methods/behaviours. Ordering the former alphabetically makes it easier to find them in my experience. E.g. trying to find the docs for Array in builtin is an annoying exercise. But enough of that.

The forelast commit was failing on windows as we seem to compile our c sources as C++ somehow. Gotta investigate the arm build error.

@SeanTAllen
Copy link
Member

SeanTAllen commented Nov 4, 2022

I think the arm build error was the instance being rescheduled and then it looks like it didnt restart? I've restarted it as that is what it looks like happened to me.

Normally, when an instance is rescheduled, it gets restarted, I have however seen it not restart from time to time.

@SeanTAllen
Copy link
Member

@mfelsche i ran it and it passed.

@mfelsche
Copy link
Contributor Author

mfelsche commented Nov 4, 2022

I did remove the sorting change.

@SeanTAllen
Copy link
Member

So if this requires a specific theme, should we be updating the help output for --docs? I'm thinking no, but as you mentioned, having a section in the tutorial would be good.

@mfelsche
Copy link
Contributor Author

mfelsche commented Nov 4, 2022

I will add a section about generating documentation to the tutorial. 👍

@SeanTAllen
Copy link
Member

Before we merge this, I'd like to add some(thing) around making sure that docgen is working as part of either nightlies or PRs were we verify that the docs build. It would probably be just a nightly job that verifies everything builds but doesn't update anything. I'll get to that soon-ish. At the moment, I'm sick and feverish so it will be at least a bit.

@mfelsche are you ok with the delay?

@mfelsche
Copy link
Contributor Author

mfelsche commented Nov 5, 2022

Of course I am! Take all the time you need and most importantly get well soon!
You can also point me at the place where such a job would be implemented in your mind, and i do the typing.

@SeanTAllen
Copy link
Member

@mfelsche the .ci-dockerfiles/stdlib-builder should be updated as part of this PR, yes?

@SeanTAllen
Copy link
Member

@mfelsche
Copy link
Contributor Author

mfelsche commented Nov 5, 2022

@mfelsche the .ci-dockerfiles/stdlib-builder should be updated as part of this PR, yes?

It is, i updated it to install the necessary mkdocs-metarial theme.

The https://github.com/ponylang/library-documentation-action should also be updated.

Will do so.

@SeanTAllen
Copy link
Member

Here's a PR to add a nightly documentation build...

#4229

@SeanTAllen
Copy link
Member

Two small changes please @mfelsche

The release notes, please have no newlines in paragraphs. Then it will look good in whatever medium it is displayed (like on github and elsewhere).

Two, this is a breaking change so can you add to the release notes to note the change that users will have to do (installing material theme, not installing the old mkdocs-ponylang) in order to have everything continue to work.

Once those updates are made, I'll merge this.

@SeanTAllen
Copy link
Member

I see you fixed the Windows issue in the PR. Given this PR fixes two different things please do the following.

Add release notes for each fix directory to "next-release.md" and remove the interim release notes file.
Add changelog entries for each change (mkdocs material and windows fix) directly to the CHANGELOG in the appropriate places with the appropriate formatting with both linking to this PR.

Or,

please remove the Windows fixes from this PR and open another for it.

Your choice. Let me know which you decide you want to do.

As the ponylang-mkdocs-theme wasn't maintainable anymore.
This puts all the necessary tweaks inside the generated files (logo, custom css, theme adaptions and extensions)
and only depends on the user to have mkdocs and the python package material-mkdocs installed to generate and serve html docs.

This also contains an improvement to sort items in a package alphabetically to make it easier to find classes/actors/primitives.
Signed-off-by: Matthias Wahl <matthiaswahl@m7w3.de>
@mfelsche
Copy link
Contributor Author

mfelsche commented Nov 7, 2022

I chose the path of pain (1).

CHANGELOG.md Outdated
@@ -8,13 +8,14 @@ All notable changes to the Pony compiler and standard library will be documented

- Avoid fairly easy to trigger overflow in Windows Time.nanos code ([PR #4227](https://github.com/ponylang/ponyc/pull/4227))
- Fix incorrect interaction between String/Array reserve and Pointer realloc ([PR #4223](https://github.com/ponylang/ponyc/pull/4223))
- Fix broken docgen on Windows ([PR #4226](https://github.com/ponylang/ponyc/pull/4223))
Copy link
Member

Choose a reason for hiding this comment

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

For some reason, I can't suggest a change anymore. No idea why. The URL points to the wrong PR.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of docgen, can we say "documentation generation"


This is a **Breaking Change** in so far that users that want to generate documentation with ponyc now have to install the python package `mkdocs-material` instead of `mkdocs-ponylang` to generate their documentation in HTML form with mkdocs. The [library documentation github action](https://github.com/ponylang/library-documentation-action) will have the correct dependencies installed, so no changes should be needed when using it.

## Fix broken docgen on Windows
Copy link
Member

Choose a reason for hiding this comment

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

Can we call it documentation generation for this release note?

@@ -16,3 +16,12 @@ This could lead to very surprising results if `reserve` had been called by `unde

In order to make documentation on a class, actor, primitive or struct easier to find, they are now sorted alphabetically inside their package.

## Adapt docgen to only depend the mkdocs material theme

We used to have our own mkdocs theme for ponylang documentation, it was based off of the mkdocs-material theme. This change puts the adaptationswe need into the mkdocs.yml file itself, so we don't depend on our own theme anymore, but only on the upstream mkdocs-material.
Copy link
Member

Choose a reason for hiding this comment

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

"adapationwe" is a typo.

@@ -16,3 +16,12 @@ This could lead to very surprising results if `reserve` had been called by `unde

In order to make documentation on a class, actor, primitive or struct easier to find, they are now sorted alphabetically inside their package.

## Adapt docgen to only depend the mkdocs material theme
Copy link
Member

Choose a reason for hiding this comment

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

Can we call it "documentation generation" rather than docgen?

CHANGELOG.md Outdated
### Changed

- Sort package types in documentation ([PR #4228](https://github.com/ponylang/ponyc/pull/4228))
- Adapt docgen to use the mkdocs material theme ([PR #4226](https://github.com/ponylang/ponyc/pull/4226)
Copy link
Member

Choose a reason for hiding this comment

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

Can we call it documentation generation rather than "docgen"?

Copy link
Member

Choose a reason for hiding this comment

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

Still have this one to update.

@SeanTAllen SeanTAllen removed the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Nov 7, 2022

## Fix broken documentation generation on Windows

Generating documentation has been broken on windows: `ponyc` was not able to write the documentation files, due to path handling issues on windows. This is now fixed and documentation generation is working again on windows.
Copy link
Member

Choose a reason for hiding this comment

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

Small one I see. windows should be Windows in a few places in here.

CHANGELOG.md Outdated
@@ -8,13 +8,14 @@ All notable changes to the Pony compiler and standard library will be documented

- Avoid fairly easy to trigger overflow in Windows Time.nanos code ([PR #4227](https://github.com/ponylang/ponyc/pull/4227))
- Fix incorrect interaction between String/Array reserve and Pointer realloc ([PR #4223](https://github.com/ponylang/ponyc/pull/4223))
- Fix broken docgen on Windows ([PR #4226](https://github.com/ponylang/ponyc/pull/4226))
Copy link
Member

Choose a reason for hiding this comment

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

Can we call it documentation generation here as well?

@SeanTAllen SeanTAllen merged commit a52451b into main Nov 8, 2022
@SeanTAllen SeanTAllen deleted the docgen-theme-embed branch November 8, 2022 00:36
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Nov 8, 2022
@SeanTAllen
Copy link
Member

SeanTAllen commented Nov 8, 2022

I merged this. If it passes the "works as expected test" tomorrow night when it goes into a nightly then I will note that the library documentation action PR should be merged on the next release.

@mfelsche is there a tutorial change open yet?

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss during sync Should be discussed during an upcoming sync documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants