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 --full-docs-dir regression #175

Merged
merged 3 commits into from
Aug 30, 2023
Merged

Fix --full-docs-dir regression #175

merged 3 commits into from
Aug 30, 2023

Conversation

CasperWA
Copy link
Collaborator

@CasperWA CasperWA commented Aug 30, 2023

Try to only use PurePosixPath as a middle stage towards str or Path.

Fixes #174

Test full_docs_folder input:

  • Test an individual folder name and that all files in the folder get the special option added.
  • Test a subfolder (using POSIX path notation (forward slashes)), ensuring it also works in Windows via the CI tests.

Try to only use PurePosixPath as a middle stage towards str or Path.
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Merging #175 (ac5cfc7) into main (cca467e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #175   +/-   ##
=======================================
  Coverage   71.42%   71.42%           
=======================================
  Files           9        9           
  Lines         567      567           
=======================================
  Hits          405      405           
  Misses        162      162           
Files Changed Coverage Δ
ci_cd/tasks/api_reference_docs.py 64.58% <100.00%> (ø)

@CasperWA CasperWA requested a review from sygout August 30, 2023 06:45
@CasperWA CasperWA marked this pull request as ready for review August 30, 2023 06:45
@sygout
Copy link
Contributor

sygout commented Aug 30, 2023

Do we have some template repos for testing the changes?

@CasperWA
Copy link
Collaborator Author

Do we have some template repos for testing the changes?

Not really. But I can point to this branch for a CI run in the repository where I noticed the error?

@sygout
Copy link
Contributor

sygout commented Aug 30, 2023

Do we have some template repos for testing the changes?

Not really. But I can point to this branch for a CI run in the repository where I noticed the error?

Ok. I was just thinking that it would be nice to have some reference repos where we can test the changes in the CI/CD prior to merge but maybe it is not really possible in practice?

@CasperWA
Copy link
Collaborator Author

Ok. I was just thinking that it would be nice to have some reference repos where we can test the changes in the CI/CD prior to merge but maybe it is not really possible in practice?

No, it's a great idea! I've been thinking the same. But I've come to the conclusion that if it's not easily managed and also doesn't include a major part of the use cases and edge cases it's simpler/better to keep the actual repositories as "reference repos".

In reality, the CI workflows should be where we test these things through reference repos and similar, which might also be possible with a fancy setup, but (for me at least) it would take a rather long time to setup in a satisfactory way.

I think the main issue for me goes back to the question, when everything has been setup and is running, whether it will then properly represent all relevant use and edge cases. And I'm not convinced of that case.

@sygout
Copy link
Contributor

sygout commented Aug 30, 2023

Thanks for the explanation and good that it is recorded in the PR. I will approve it now.

@CasperWA
Copy link
Collaborator Author

CasperWA commented Aug 30, 2023

Anyway - for this specific issue, I can test it locally in different ways. The one I've chosen is to use this branch as the pre-commit hook revision in the relevant repository (in this case OPTIMADE Gateway), where I found the regression.
Using this branch to run the hook the regression is fixed.

Note, this is when running on a WSL 2 Ubuntu system with Python 3.8.10.

@CasperWA CasperWA merged commit cd79149 into main Aug 30, 2023
@CasperWA CasperWA deleted the cwa/fix-174-full-docs-dir branch August 30, 2023 08:52
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.

Regression in --full-docs-dir input
2 participants