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: solve the test errors #8

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

fix: solve the test errors #8

wants to merge 18 commits into from

Conversation

12rambau
Copy link
Contributor

@12rambau 12rambau commented Oct 6, 2023

Sorry for the very noisy PR but act is not able to locally emulate windows and mac runners so I was forced to push everything to here to check test success. here is a little summary on what this is doing:

  • create a devcontainer configuration to work in github codespaces
  • from this devcontainer you can now run the tests by running act -j test
  • solve the windows issue y creating a platform dependent expected output
  • solve the mac issue by reading data from the zip file Fix Consider using the zip file on macOS #2
  • update uuid because npm was complaining too much
  • as I was forgetting it all the time I took the liberty of adding a pre-commit hook that will run the lint and build scripts every time you commit.

@12rambau 12rambau marked this pull request as ready for review October 6, 2023 09:04
@@ -29,6 +31,7 @@
},
"devDependencies": {
"@types/node": "^20.8.0",
"pre-commit": "^1.2.2",
"prettier": "^3.0.3",
Copy link
Owner

Choose a reason for hiding this comment

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

I think there is a better solution for this. I'm going to put the node_modules and the build artifacts to a separate, orphaned dist-branch, which will also receive the tags. This branch could be built by the CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you want me to remove all the pre-commit related statements then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it a second though, we have 2 options here:

  1. build the actions and push .js and node_module to dist (current)
  2. build on a CI branch that will be wired to the release

I agree solution 2 is better but as you mentioned it will be done in another PR.
So keep things working while you work on it, the pre-commit hook is the only guarantee to have consistently .ts and .js files aligned. In short I would argument in favor of keeping it until you create the extra CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any chance to see this PR updated? I would prefer to rely on your repository instead of my fix branch in the pydata-sphinx-theme.

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.

Consider using the zip file on macOS
2 participants