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

Convert deprecation policy and maintainer guide to markdown #11218

Merged
merged 9 commits into from
Nov 20, 2023

Conversation

javabster
Copy link
Contributor

Summary

This PR creates markdown versions of the deprecation policy and maintainer guide, so we can safely remove https://qiskit.org/documentation/deprecation_policy.html and https://qiskit.org/documentation/maintainers_guide.html

Details and comments

@javabster javabster requested a review from a team as a code owner November 8, 2023 22:22
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Nov 8, 2023

Pull Request Test Coverage Report for Build 6935485297

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 10 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 85.887%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 92.17%
crates/qasm2/src/parse.rs 6 97.13%
Totals Coverage Status
Change from base Build 6935431635: 0.01%
Covered Lines: 66359
Relevant Lines: 77263

💛 - Coveralls

Eric-Arellano
Eric-Arellano previously approved these changes Nov 9, 2023
Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thank you, Abby!

DEPRECATION.md Outdated Show resolved Hide resolved
DEPRECATION.md Outdated Show resolved Hide resolved
DEPRECATION.md Outdated Show resolved Hide resolved
DEPRECATION.md Outdated Show resolved Hide resolved
DEPRECATION.md Outdated Show resolved Hide resolved
DEPRECATION.md Outdated Show resolved Hide resolved
DEPRECATION.md Outdated Show resolved Hide resolved
DEPRECATION.md Outdated Show resolved Hide resolved
MAINTAINING.md Show resolved Hide resolved
Eric-Arellano
Eric-Arellano previously approved these changes Nov 13, 2023
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This generally seems fine to me in principle, although I do think that it's a regression to move from hosting this in a properly rendered documentation build GitHub markdown views.

Should this PR also remove the old versions of these files?

There are other open PRs that are modifying the related files here, such as #11205, which have logical conflicts.

DEPRECATION.md Outdated Show resolved Hide resolved
DEPRECATION.md Outdated
Comment on lines 39 to 40
- The alternative path must be in place for one minor version before any
warnings are issued. For example, if we want to replace the function `foo()`
Copy link
Member

Choose a reason for hiding this comment

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

In all the bullets in this list, the continuation-line indentation looks like it's got an extra space in it. Not that it affects the display, it just makes it a bit harder to update.

DEPRECATION.md Outdated Show resolved Hide resolved
DEPRECATION.md Show resolved Hide resolved
DEPRECATION.md Outdated
*migration guide* might be needed. Please write these guides in Qiskit's documentation at
https://github.com/Qiskit/documentation/tree/main/docs/api/migration-guides. Once
the migration guide is written and published, deprecation
messages and documentation should link to it (use the `additional_msg: str` argument for
Copy link
Member

Choose a reason for hiding this comment

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

Minor style: I don't think we need to include the type hint for an argument when mentioning it in prose.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added this a few months ago. I think it's useful for people to know what the argument is referring to. I agree it's not "necessary", but I think it's useful for only 5 extra characters.

Copy link
Member

Choose a reason for hiding this comment

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

I'd have thought the API documentation is a more appropriate place for this. To me it adds noise and makes it harder to scan (especially because this is the second time it happens).

MAINTAINING.md Outdated
Comment on lines 48 to 49
Refer to https://qiskit.github.io/qiskit_sphinx_theme/apidocs/index.html for how to create and
write effective API documentation, such as setting up the RST files and docstrings.
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 still going to be a relevant link after the full 1XP migration?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. This site will still exist for the sake of the Qiskit Ecosystem. Its guidance on API docs should still be relevant for Qiskit itself.

@Eric-Arellano
Copy link
Collaborator

Should this PR also remove the old versions of these files?

Not yet. We're going to set up redirects from the qiskit.org pages to these markdown files as part of Qiskit/documentation#216 (still WIP!)

@jakelishman
Copy link
Member

I think this PR should still remove the files from this repository; Qiskit/documentation should just not deploy the removal until its redirects are set up. Doing it split in this repo means there's more time for the files to get out of sync without detection (as it stands, the linked PR above won't detect a merge conflict because this PR doesn't remove the old files), and because this repo should have its docs in a consistent state.

@Eric-Arellano
Copy link
Collaborator

I think this PR should still remove the files from this repository;

I realize that will only impact qiskit.org/documentation/dev and not the current docs. So that works for me.

Co-authored-by: Jake Lishman <[email protected]>
@javabster
Copy link
Contributor Author

The only reason I'd see for not removing the files yet in this PR is in case we need to do any emergency qiskit.org docs deployments between now and the redirects kicking in on 29th Nov. If we're fine risking that then I'll remove the files, I don't have a strong opinion either way.

At some point (out of scope for this PR) we should also remove the old tutorials and how-to guides etc as well, they've all been either migrated or superseded by 1docs content. We can check the commit history to see if anything was added since the migration or 0.45 release that isn't in 1docs


contributing_to_qiskit
deprecation_policy
maintainers_guide
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to delete this file and contributing_to_qiskit.rst too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah jake asked me to remove it in the other contributing guide PR as well so just thought i'd do it in both to avoid merge conflicts whichever gets merged first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-11-17 at 16 40 27 although i realise now that plan has backfired 🫠

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this PR will want to delete the maintainers guide and deprecation policy, but leave contributing guide. You'll get a merge conflict with your PR, but such it is 🤷

jakelishman
jakelishman previously approved these changes Nov 17, 2023
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, Abby. Similar to #11217, if you're worried about not being ready for this change on the backend yet, shall we leave this approved but unmerged til you're ready?

Having the old files removed in the PR means we can't accidentally lose any changes from other PRs to the old file, because git will force us to resolve a merge conflict, so we fail safe in that case.

(btw, I know there's a docs-build failure just because of the contributing_to_qiskit.rst file being orphaned at the moment - it's just an approval in spirit, rather than knowing we're immediately going to merge, since I assume #11218 will go first.)

@jakelishman jakelishman added this pull request to the merge queue Nov 20, 2023
Merged via the queue into Qiskit:main with commit 45e4252 Nov 20, 2023
14 checks passed
github-merge-queue bot pushed a commit to Qiskit/documentation that referenced this pull request Nov 22, 2023
This PR replaces any qiskit.org links with 1XP or github equivalents.
Need to wait to merge until I can update the deprecation policy and
maintainer guide links after
Qiskit/qiskit#11218 merges.

This should partially resolve #297 , however for the api links we'll
need to make updates for those directly in the repos where the API refs
live (maybe this is beyond the scope of the original issue?)

---------

Co-authored-by: Eric Arellano <[email protected]>
FabianBrings pushed a commit to FabianBrings/qiskit that referenced this pull request Nov 27, 2023
…1218)

* Converted deprecation policy and maintainer guide to markdown

* Apply suggestions from code review

Co-authored-by: Eric Arellano <[email protected]>

* Added more context around writing docs

* Update DEPRECATION.md

Co-authored-by: Jake Lishman <[email protected]>

* fixed some formatting issues

* Removed deprecation_policy.rst and maintainers_guide.rst

* Apply suggestions from code review

Co-authored-by: Jake Lishman <[email protected]>

* update index.rst

---------

Co-authored-by: Eric Arellano <[email protected]>
Co-authored-by: Jake Lishman <[email protected]>
frankharkins pushed a commit to frankharkins/documentation that referenced this pull request Jul 22, 2024
This PR replaces any qiskit.org links with 1XP or github equivalents.
Need to wait to merge until I can update the deprecation policy and
maintainer guide links after
Qiskit/qiskit#11218 merges.

This should partially resolve Qiskit#297 , however for the api links we'll
need to make updates for those directly in the repos where the API refs
live (maybe this is beyond the scope of the original issue?)

---------

Co-authored-by: Eric Arellano <[email protected]>
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.

5 participants