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

Reorganize the changelog and the way it's displayed in the doc #6688

Closed

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented May 24, 2022

Type of Changes

Type
✨ New feature
🔨 Refactoring
📜 Docs

Description

This is a work in progress to discuss the reorganisation of the changelog. TODO:

Closes #5728

@Pierre-Sassoulas
Copy link
Member Author

@DanielNoord this is a draft but please let me know what you think of the new TOC for the changelog :)

@coveralls
Copy link

coveralls commented May 24, 2022

Pull Request Test Coverage Report for Build 2454606962

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.525%

Totals Coverage Status
Change from base Build 2454187534: 0.0%
Covered Lines: 16350
Relevant Lines: 17116

💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the store-old-changelog branch 2 times, most recently from 2bd64ce to ea0c1b2 Compare May 24, 2022 14:22
@DanielNoord
Copy link
Collaborator

I like the separation of the different major version in the sidebar.

However, I don't really like how there are 3/4 different pages for one version. Taking 2.13 as example. There is:
https://pylint--6688.org.readthedocs.build/en/6688/whatsnew/2/2.13/index.html#
https://pylint--6688.org.readthedocs.build/en/6688/whatsnew/2/2.13/summary.html
https://pylint--6688.org.readthedocs.build/en/6688/whatsnew/2/2.13/full.html

The amount of pages also doesn't really correspond with the amount of links in the sidebar. Which is somewhat confusing.
Besides, what does the https://pylint--6688.org.readthedocs.build/en/6688/whatsnew/2/2.13/index.html page offer besides the sidebar? We can add the warning in a .. note:: directive at the top of each page and then we can remove the extra click we need to do now.

My suggestion would be one page for every minor version which starts with the current Changelog (so the different sections) and then just all what's new under it in the same page. By using different arguments in get_rst_title we can create different header levels so that those headers aren't all so big.

@Pierre-Sassoulas
Copy link
Member Author

It's not generated I did everything manually, so I can't use 'get_rst_title' maybe we could transform the changelog to JSON so it's easier to generate and we have proper link to issues.. ? I'm not sure it's worth it though.

We'd need a one off script to convert to JSON and a script to generate from JSON for a changelog that would never change. Maybe we could use the JSON for everything even the new changelogs ?

@DanielNoord
Copy link
Collaborator

It's not generated I did everything manually, so I can't use 'get_rst_title' maybe we could transform the changelog to JSON so it's easier to generate and we have proper link to issues.. ? I'm not sure it's worth it though.

We'd need a one off script to convert to JSON and a script to generate from JSON for a changelog that would never change. Maybe we could use the JSON for everything even the new changelogs ?

Ah, we could also just change the headings. Sphinx decides the headers based on the order they occur on.
So if we do:

2.13
###
2.13.0
-----
New checkers
^^^^^^^^^^^

We will get different <h> tags. That should be fairly easy to do with a replace all in the changelog files.

@Pierre-Sassoulas
Copy link
Member Author

Let's work the structure on 2.13 only then when we're satisfied let's do the rest.

@Pierre-Sassoulas
Copy link
Member Author

I pushed a new version with only 2.13 modified, not very satisfied to be honest, let me know what you think :).

@DanielNoord
Copy link
Collaborator

I think this is definitely an improvement. Only thing I would change is to remove this page:
https://pylint--6688.org.readthedocs.build/en/6688/whatsnew/2/2.13/index.html
And make that go to:
https://pylint--6688.org.readthedocs.build/en/6688/whatsnew/2/2.13/content.html
immediately.

@Pierre-Sassoulas
Copy link
Member Author

Do you know how we can still have the dropdown for the link ? (We have a flat link and no navigation in the left toc bar if we do that.

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas I think a :hidden: toctree directive on the content.html page should display in the index. The issue is that content.html doesn't have toctree so Sphinx doesn't know what to put as links there I think.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented May 26, 2022

All right, I fixed 2.13 and modified 2.12 too, I prefer the 2.12 version because the full changelog and summary are separated like they were before (It's handy when there is a duplicated link so we don't have to modify the current changelog to avoid an error, but also make sense as they were supposed to be separated). I think otherwise it's identical. Let me know what you think :)

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the store-old-changelog branch 2 times, most recently from 5478e5d to e32ef86 Compare May 27, 2022 05:59
@Pierre-Sassoulas
Copy link
Member Author

I think we should merge the two first point of the TODO so we don't have rebase conflict when something new end up in 2.14 changelog while we figure out the new changelog.

@DanielNoord
Copy link
Collaborator

All right, I fixed 2.13 and modified 2.12 too, I prefer the 2.12 version because the full changelog and summary are separated like they were before (It's handy when there is a duplicated link so we don't have to modify the current changelog to avoid an error, but also make sense as they were supposed to be separated). I think otherwise it's identical. Let me know what you think :)

I don't really see the difference between 2.13 and 2.12?

I was thinking of removing the index.html page and only keep content.html is that a possibility you think?

@Pierre-Sassoulas
Copy link
Member Author

I don't really see the difference between 2.13 and 2.12?

In 2.12 the full changelog and what's new's summary are each on their own page. (In 2.13 I had to merge the two and fix an issue with a link that was duplicated).

I was thinking of removing the index.html page and only keep content.html is that a possibility you think?

It's making a flat link on the left TOC, I've tried for maybe 30mn before giving up 😄

@DanielNoord
Copy link
Collaborator

In 2.12 the full changelog and what's new's summary are each on their own page. (In 2.13 I had to merge the two and fix an issue with a link that was duplicated).

I like the single page better. Whenever you're opening old changelogs you're likely looking for something specific. So being Abel to search on one page instead of two is better imo.

It's making a flat link on the left TOC, I've tried for maybe 30mn before giving up 😄

Hm, that's annoying. I might have a look as well.

@Pierre-Sassoulas
Copy link
Member Author

So being Abel to search on one page instead of two is better imo.

But the full changelog contain everything, right ? So you can search in the full changelog only and get your result, (or on the full website with the global search)

@DanielNoord
Copy link
Collaborator

So being Abel to search on one page instead of two is better imo.

But the full changelog contain everything, right ? So you can search in the full changelog only and get your result, (or on the full website with the global search)

Yeah, I just don't understand while we would want to separate related information across multiple pages/files. With this change you also lose the ability to search within the CHANGELOG file and would need to do that with your IDE or by grepping. I consider that a little less user-friendly.
But if you want to keep separating them let's do that then.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented May 28, 2022

Yeah, I just don't understand while we would want to separate related information across multiple pages/files.

  • Keeping thing separated means adding a section and changing the header level which is quite fast. Merging means reorganizing the files, copy pasting to merge the file, changind the header level, removing the old file (there will be conflict for 2.14 if something is merged meanwhile on main), then there can be issues when we merge two files together for example if two link are identical which happens because we copy paste between the changelog and whatsnew (and we have 18 files to merge). It's a lot of work
  • It's possible to search on ReadtheDoc directly and dev probably have an IDE anyway
  • Keeping thing separated make historic sense and the data would be more organized at the end.

I'm going to apply 2.12 example everywhere and open another MR for this so we can merge asap an stop having conflict on Changelog/2.14/full.rst

@Pierre-Sassoulas
Copy link
Member Author

Blocked by #6735

@Pierre-Sassoulas Pierre-Sassoulas added the Blocked 🚧 Blocked by a particular issue label Jun 1, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.0 milestone Jun 1, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Jun 2, 2022
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as ready for review June 2, 2022 05:50
@@ -42,3 +42,5 @@ Other Changes

Internal changes
================

* Added a check in CI for changelogs entry (#6688)
Copy link
Member Author

Choose a reason for hiding this comment

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

The question here is should we keep the old format with:

* Fix a regression where messages with dash are not fully parsed

  Closes #3604

Or is * Fix a regression where messages with dash are not fully parsed (#3604) better ? Sometime we might want to be more detailed use multiple line ? Note that #3604 was an issue number but we would use the PR number now (It was just a commit at the time ffb354a but now we can't push to main so there's always a PR)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep the old system. It brings some uniformity to all changelogs and is also easier to parse the data programatically.

@Pierre-Sassoulas
Copy link
Member Author

The follow-up will be #6781 but adding the CI check asap is important imo.

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas I'll review this in a minute, but could you look into #6787 (comment)
pre-commit is broken locally and I can't commit anything without no-verifying.


on:
pull_request:
types: [opened, synchronize, labeled, unlabeled, reopened]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there anything we exclude here? Can't it just be on: pull_request?

Copy link
Member Author

Choose a reason for hiding this comment

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

It needs to have labeled and unlabeled because if we add or remove "skip news" the pipeline result is not the same.

types: [opened, synchronize, labeled, unlabeled, reopened]

permissions:
contents: read
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is automatic.

@@ -42,3 +42,5 @@ Other Changes

Internal changes
================

* Added a check in CI for changelogs entry (#6688)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep the old system. It brings some uniformity to all changelogs and is also easier to parse the data programatically.

@jacobtylerwalls jacobtylerwalls removed their request for review June 7, 2022 12:46
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉This comment was generated for commit 8d2d094

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jun 8, 2022

I'm closing this in favor of #6875. @DudeNr33 you could take some inspiration from the changelog pipeline, I think instead of a regex to check on the changelog we need to check that the description exists in the issue but everything around it could be the same. the other script is a one time script to fix the old changelog, we don't need to merge it as it's not going to be used for new changelogs.

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.15.0 milestone Jun 8, 2022
@Pierre-Sassoulas Pierre-Sassoulas deleted the store-old-changelog branch July 21, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A single changelog file instead of two
5 participants