-
Notifications
You must be signed in to change notification settings - Fork 54
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
Move doc files from main repo into docs repo #426
base: main
Are you sure you want to change the base?
Conversation
9680f12
to
f1cfcdd
Compare
f1cfcdd
to
3d08aaf
Compare
@@ -69,7 +69,7 @@ filtered and produces an error, if found. | |||
suite. To manually check the generated spec file, run | |||
|
|||
```bash | |||
rucio/tools/test/check_rest_api_documentation.sh FILE | |||
rucio_documentation/tools/check_rest_api_documentation.sh FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should say this to be explicit
rucio_documentation/tools/check_rest_api_documentation.sh FILE | |
[rucio documentation folder]/tools/check_rest_api_documentation.sh FILE |
rucio_documentation
is clear enough imo, but it might be interpreted as if we have an actual folder named rucio_documentation
, whereas [rucio documentation folder]
might be more easily interpreted as 'top-level Rucio documentation folder'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're linking directly to the file anyways, is there any harm in going Full explicit and doing
rucio_documentation/tools/check_rest_api_documentation.sh FILE | |
[rucio/documentation](https://github.com/rucio/documentation)/tools/check_rest_api_documentation.sh FILE |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, but I feel that the /
in rucio/documentation
could be interpreted as being part of the path, so it might be confusing
from apispec import APISpec # type: ignore | ||
from apispec_webframeworks.flask import FlaskPlugin # type: ignore | ||
from rucio.vcsversion import VERSION_INFO # type: ignore | ||
from rucio.web.rest.flaskapi.v1.main import application # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are the # type: ignore
comments for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The python script checking action claims these packages don't exist and fails the build 😔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add them to the requirements instead - I guess they can be seen as part of the requirements for building the docs, so it makes sense that they'd need to be part of the requirements.txt
file
check_python_scripts:
name: Check Python Scripts
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4
- name: Install dependencies
run: |
pip install -r tools/requirements.txt
- name: Run black
run: |
black --check .
- name: Run isort
run: |
isort --profile black --filter-files --check .
- name: Run flake8
run: |
flake8 --config tools/.flake8
- name: MyPy
run: |
mypy --ignore-missing-imports .
View issue rucio/rucio#7278 ; Removing check_rest_api_documentation from that repo and moving here as part of decommissioning of the docs test suite