Closed sadielbartholomew closed 1 year ago
About the changes to the repo - I'd avoid commiting the generated files (HMTL, js and so on). I can't think of any truly harmful consequence of it right now though - but it makes checking out the repo a lot bigger unecessarily: there is no point modifying the built site without changing the sources - so there is no point of tracking changes to it.
Instead I'd just commit the recipes (Makefile, make.bat) and the content of the source
directory, along with any static stuff like images. When deploying through GH pages I usually have (inspired from many other examples out there) a github action running sphinx and pushing the generated site to the gh-pages
branch of the repo which can be picked automatically by the GitHub pages service. Here is an example. I believe it's a common way of doing this: that's actually what's suggested on the sphinx docs.
I'm not against merging this PR as it is now and remove the generated files later on, at the same time of adding a github action deploying the site. It's something I can do if you're running of time @sadielbartholomew - but will not unless explicitly assingned to it.
Thanks for your review @tlestang! And I am glad you are happy with the structuring and style.
What you say regarding the built content, namely:
I'd avoid commiting the generated files (HMTL, js and so on).
makes a lot of sense, and I am happy to do that (and it is easy enough by reverting the two self-contained commits where I push and then update the built content, only). I wasn't familiar with the use of CI jobs to run the build to generate the content from the source content and push that to hosting of some sort, but now you specify this is possible, it does seem like a better solution in many ways to committing the content built locally! In particular I fully agree that:
there is no point modifying the built site without changing the sources - so there is no point of tracking changes to it
Thanks also for the example you have referenced, which I will have a look at.
First of all, I'll revert the few commits which push the built content. I'll do that now. Then I can try to set up the Actions workflow to create and host the built content, though I wonder when you mention:
I'm not against merging this PR as it is now and remove the generated files later on, at the same time of adding a github action deploying the site.
that maybe the task of adding the Action to the repo might better off as a new, follow-on, PR to this one which will add all of the source content, to keep those two sub-tasks separate (especially since I know it can be a bit of a pain to get an workflow to initially run in the intended way)?
@tlestang and all: the reversion of all of the built content is complete with my latest commit, which squashed two commits which each reverted one of the pair of commits which added and then updated the built content. This reduces the file and line count for review considerably, amongst all of the other benefits going forward :)
Please let me know (Thibault and anyone else with thoughts) whether you would prefer that I try to set up a GitHub Action workflow to generate and host the built content here on this PR, or on a follow-on PR after. My preference is to do the latter and use a separate PR. In which case, this is ready for (re-)review...
Thanks @sadielbartholomew -- sorry for the slow reply. This all looks good to me! Personally happy for you to press the green button. I suggest this is "squashed and merged" into a single commit to main
.
Curiosity: what are the files in _static/favicon
and docs/.no_jekyll
for?
Thanks @tlestang for your further comments.
So I take it (sorry, I can't remember the procedure we agreed to as a group) that one review is sufficient to merge? I'm happy enough to wait for anyone else to comment, whether they do a full-blown review or not, but equally I'd be comfortable to go ahead and squash-merge based on your review alone.
@GreenScheduler/hack-day-team (by which I mean everyone, this formal GitHub 'team' is incomplete and has only six of us, but I don't know how to alert everyone without listing everyone's names...) : if anyone plans to comment or review, please let us know ASAP so we can get a feel for whether anyone else can or wants to review this...
I suggest this is "squashed and merged" into a single commit to main.
I am happy to squash, though given this PR and addition of separate documentation entails quite a few different self-contained aspects, I'd prefer to squash into more than one commit, probably ~5 or so would be most sensible in my view. I can do that shortly.
Curiosity: what are the files in _static/favicon and docs/.no_jekyll for?
The favicon
files add the CATS DALLE image (which I am taking as a logo for the project, for now at least as this can be agreed at a later date if an alternative would be better) as the favicon across the docs pages (as set in the conf.py
). A favicon is a little icon appearing with the page name on a tab marker of the browser, if you aren't familiar. I thought we might as well set one as it is easier and contributes to a consistent theme (they need to be 16x16 so I used an online generator to create the appropriately-sized images from the CATS DALLE image).
The .nojekyll
is an empty file which tells Github not to try and build the site with Jekyll (the default) which in practice is needed because without it GitHub Pages won't add any images or CSS styling since they are contained in directories beginning with underscores (_static
, _templates
, _images
, etc.) which Jekyll ignores because it assigns special meaning to that name pattern.
Many apologies - I've been up to my eyeballs here again.
This all looks sensible to me. Adding the deploy to github pages action would make it easer to look at the documentation but the structure and content looks good. Unless you have a burning desire to do that, it's probably worth merging now and dealing with that later. I wouldn't worry about squashing all the commits together.
I think in general one review is enough, but I'll stick that on the agenda for next week.
Thanks for the review @andreww and no worries at all about timing, I realise we are all contributing to this around our day-to-day work.
Adding the deploy to github pages action would make it easer to look at the documentation but the structure and content looks good. Unless you have a burning desire to do that, it's probably worth merging now and dealing with that later. I wouldn't worry about squashing all the commits together.
OK, thanks. In that case I will merge this now, ready to get that GitHub Action created ASAP (I'll open an Issue). If someone else has experience adding the relevant workflow to a repo to generate and host the built docs pages, they would be welcome to do it, but equally I am happy to do it, though not fully sure at what point I'll be able to find the time - perhaps next week, but I can't be certain. So if anyone feels keen to do that before I get round to it, they can assign themselves to the Issue and claim that task.
I think in general one review is enough, but I'll stick that on the agenda for next week.
Good idea, it feels like an important practical consideration to know going forward. Maybe for PRs which touch the functional code in a more significant or structural way, we can ask for two approvals, but I agree that one sounds good to me for most if not all PRs.
Resolve #39 by adding Sphinx documentation. This PR commits all of, under an umbrella
docs/
directory:README.md
document.Reviewers: you can preview the documentation via my branch here at: https://sadielbartholomew.github.io/cats/build/html/index.html. [NOTE: link now broken due to removal of built docs - preview will be available again on a different branch shortly.] I advise you only need to review the source content and infrastructure via the files in
source
and the built content via viewing the webpages there (unless you like looking at raw HTML code!) If and when this PR gets merged, the pages can be hosted using GitHub Pages under the 'GreenScheduler' organisation webpages i.e. the equivalent link withsadielbartholomew
->GreenScheduler
.Note: