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

fix: remove unecessary _tket perfix in local docs build #1721

Merged
merged 10 commits into from
Dec 13, 2024

Conversation

CalMacCQ
Copy link
Contributor

@CalMacCQ CalMacCQ commented Dec 12, 2024

Description

Note that this only impacts the local build. The website build is already fixed and deployed.

Updating the build script to replace all of the refernce to the internal _tket module. Previously this was done by commands in the Makefile but this stopped working due to some previous changes.

I've decided to remove the Makefile for now as it was unused. Instead I've put the sed replace in the build-docs.sh script.

Related issues

closes #1719

@CalMacCQ CalMacCQ marked this pull request as draft December 12, 2024 13:43
@CalMacCQ CalMacCQ changed the title fix: don't show _tket in the docs fix: don't show unecessary _tket in the docs Dec 12, 2024
@CalMacCQ CalMacCQ changed the title fix: don't show unecessary _tket in the docs fix: don't show unnecessary _tket in the docs Dec 12, 2024
@CalMacCQ CalMacCQ marked this pull request as ready for review December 12, 2024 14:13
@CalMacCQ CalMacCQ requested a review from cqc-alec December 12, 2024 14:13
@@ -3,6 +3,8 @@ cp -R pytket-docs-theming/quantinuum-sphinx .
cp pytket-docs-theming/conf.py .

sphinx-build -b html . build -D html_title="pytket"
find build/ -type f -name "*.html" | xargs sed -e 's/pytket._tket/pytket/g' -i ""
Copy link
Contributor Author

@CalMacCQ CalMacCQ Dec 12, 2024

Choose a reason for hiding this comment

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

slightly worried that these sed comands are not architecture independent. They work fine on my mac. Maybe this is a good reason to use a Makefile?

The website uses linux runners in C.I. so will make sure to use the linux syntax there and test that it works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could also check the platform from within the shell script and perform a switch on that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can confirm that this does not work on Linux.

Copy link
Collaborator

@cqc-alec cqc-alec left a comment

Choose a reason for hiding this comment

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

I think we need to make this work at least on Linux and MacOS.

@CalMacCQ
Copy link
Contributor Author

I think we need to make this work at least on Linux and MacOS.

Yes, happy to update the script to check the platform its running on and adjust the sed command accordingly.

@CalMacCQ CalMacCQ changed the title fix: don't show unnecessary _tket in the docs fix: remove unecessary _tket perfix in local docs build Dec 13, 2024
@CalMacCQ
Copy link
Contributor Author

I think we need to make this work at least on Linux and MacOS.

Now added some handling so that the build script should work on MacOS and Linux. I've tested locally

Note that the webiste is already fixed. This script is only for local builds

@CalMacCQ CalMacCQ requested a review from cqc-alec December 13, 2024 15:42
@CalMacCQ CalMacCQ merged commit 5f7af8d into main Dec 13, 2024
30 checks passed
@CalMacCQ CalMacCQ deleted the fix/docs_underscore branch December 13, 2024 16:00
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.

The docs show pytket._tket instead of just pytket
2 participants