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

Implement multi docs #70

Closed
wants to merge 3 commits into from
Closed

Implement multi docs #70

wants to merge 3 commits into from

Conversation

galleon
Copy link
Contributor

@galleon galleon commented Oct 7, 2021

  • Add workflow to generate doc
  • Add new dropdown widget in navbar to select doc version

@galleon galleon added the enhancement New feature or request label Oct 7, 2021
@galleon galleon requested a review from neo-alex October 7, 2021 17:05
@galleon galleon self-assigned this Oct 7, 2021
.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/docs.yml Outdated Show resolved Hide resolved
docs/.vuepress/config.js Outdated Show resolved Hide resolved
docs/.vuepress/config.js Outdated Show resolved Hide resolved
@dbarbier
Copy link
Contributor

dbarbier commented Oct 7, 2021

Why is a new workflow needed? We already have build.yml and release.yml. BTW release.yml should have been modified to not deploy.

@dbarbier
Copy link
Contributor

dbarbier commented Oct 7, 2021

There are at least 2 ways to build versioned documentation:

  • Static: available versions are hardcoded when building docs, which means that an older documentation can provide links to master and previous releases, but not future ones
  • Dynamic: all available versions are retrieved by clients. This comes at a cost, Github limits the number of API requests, so the dropdown menu may be unavailable when displaying lots of pages.

Your commit message for 5ab99ab is way too terse. You should describe the different options, and why you implemented it that way.

@galleon
Copy link
Contributor Author

galleon commented Oct 8, 2021

There are at least 2 ways to build versioned documentation:

  • Static: available versions are hardcoded when building docs, which means that an older documentation can provide links to master and previous releases, but not future ones
  • Dynamic: all available versions are retrieved by clients. This comes at a cost, Github limits the number of API requests, so the dropdown menu may be unavailable when displaying lots of pages.

Your commit message for 5ab99ab is way too terse. You should describe the different options, and why you implemented it that way.

Will improve commit description.

@galleon
Copy link
Contributor Author

galleon commented Oct 8, 2021

Why is a new workflow needed? We already have build.yml and release.yml. BTW release.yml should have been modified to not deploy.

docs.yml has been removed. Docs is generated through build.ymlwhen on master or through release.yml workflows.

@dbarbier
Copy link
Contributor

dbarbier commented Oct 8, 2021

At the moment docs.yml is still there, please push your changes before writing comments.

),
handleSelected: (input, event, suggestion) => {
const {pathname, hash} = new URL(suggestion.url);
const routepath = pathname.replace('/NotifyBC', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

scikit-decide

- generate a static web site on GH pages for each tagged release
  and for master (dev version)
- create vuejs component to choose selected version
- modify default theme to add version component in navbar
- move all vuepress related packages as dev dependencies
- fix base url do avaoid double slash
- display dev instead of main in dropbox for doc version selection
- use latest version of publishing GH action
- change target directory on GH pages for release documentation
- generate doc only for master branch
branch: gh-pages # The branch the action should deploy to.
folder: docs/.vuepress/dist # The folder the action should deploy.
target-folder: ${{ env.DOCS_VERSION_PATH }}
commit-message: Publish documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as before, there is no reason to upgrade this action in this PR, do it in a separate PR. Here you only have to add TARGET_FOLDER.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having multiple PRs might be desirable but we need to find a compromise between related work and time needed to review PRs. This actions needed to be modified and I took this opportunity to update to the latest version (same in build.yml btw).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the exact opposite, putting unrelated changes into the same PR makes review much more time consuming.

@@ -227,6 +227,7 @@ jobs:

build-docs:
needs: [test-unix, test-macos, test-windows]
if: ${{ github.ref == 'refs/heads/master' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation must also be built for PRs.
It could also be handy to temporarily deploy them, for instance this would be useful for this PR.

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 request is to generate doc for dailys based on changes on master and for releases (tagged versions). If I was to regenerate docs for each and every PR or branch this will soon be messy. Why don't we keep it simple ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Build-docs step is currently performed by all PRs. You are modifying this behavior, this is mentioned neither in commit message nor in this PR, so it is unclear whether this change is intentional or not. In any case, skipping this step is a bad idea.

"scripts": {
"docs:dev": "(cd docs && python autodoc.py) && vuepress dev docs",
"docs:build": "vuepress build docs"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not reindent while making changes.

Please do not mix different issues in the same PR; if you want to change how vuepress is launched, do it in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yarn run docs:dev is simply not working if you don't launch python autodoc.py first.

Copy link
Contributor

Choose a reason for hiding this comment

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

It had always been that way without any problem. You are making changes without explanation, this makes code review harder. Again this PR is about generating versioned documentation, and all these unrelated changes divert us from discussing the main points.

@galleon galleon closed this Oct 11, 2021
@galleon galleon deleted the gal/multi_docs branch October 11, 2021 18:51
@galleon
Copy link
Contributor Author

galleon commented Oct 11, 2021

Closed when modifying branch multi_docs on my fork.

@galleon galleon mentioned this pull request Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants