Skip to content

[skip changelog] Configure git push from CI #717

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

Merged
merged 7 commits into from
May 22, 2020
Merged

[skip changelog] Configure git push from CI #717

merged 7 commits into from
May 22, 2020

Conversation

masci
Copy link
Contributor

@masci masci commented May 21, 2020

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • The PR follows our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce?

This PR fixes the generation pipeline for versioned docs.

  • What is the current behavior?

Most of the feature is implemented in master but docs are never pushed to the gh-pages branch. Also there's a bug in the version selector component that breaks it when docs are not at the root path /.

  • What is the new behavior?

This PR implements several bugfixes:

  • versioned docs are now pushed to gh-pages

  • the docs pipeline is now triggered when changes are made to the GH workflow itself

  • The command line interface of the build.py script has been expanded to simplify the code needed from within the workflow yaml definition

  • The javascript component implementing the version selector has been fixed

  • Does this PR introduce a breaking change?

No changes to the CLI in this PR


See how to contribute

@matthijskooijman
Copy link
Collaborator

I hate to be a nag, but if the pull request template is not filled in, I would suggest removing it (or at least the parts that are apparently not relevant) and maybe providing at least a brief description of the pullrequest if the title is not enough. Doing so makes the pullrequests easier to view, and also makes the notification e-mails I get a lot easier.

While I'm here: I would also suggest adding a bit more info in commit messages. Instead of just documenting what changed, also document why it needs to change or why it is helpful, which makes it easier to review and easier to trace the origin of code later. It does seem that short commit messages are more commonly used in this repository, so this is probably more of a general remark, not specific to this PR.

One remark that is specific to this PR: It seems there is a test commit in here which probably serves to test the workflow and should be removed before merging. In such cases, I would suggest marking the PR as "draft" to prevent it from being accidentally merged.

@masci
Copy link
Contributor Author

masci commented May 21, 2020

@matthijskooijman this should be a draft, my apologies.

I hate to be a nag

Then please don't be. I'm the author of the contributing guidelines for this repo and I know I should lead by example but I'm also going above and beyond to deliver this feature while swamped. I think I've done enough for this project to deserve more than a 20 minutes buffer before getting so much criticism about my work.

@masci masci marked this pull request as draft May 21, 2020 11:21
@matthijskooijman
Copy link
Collaborator

but I'm also going above and beyond to deliver this feature while swamped.

My comment was meant as friendly and constructive feedback. I can understand that time pressure or other circumstances sometimes force suboptimal choices to be made (and also make feedback look like harsher criticism than it was intended), but I could not tell that from here, of course. Apologies if my comment offended you, that was genuinely not the intention.

Anyway, good luck with your workload, and thanks for all the work you've put in already!

@masci masci added topic: documentation Related to documentation for the project component/tooling labels May 21, 2020
@masci masci marked this pull request as ready for review May 21, 2020 14:56
@masci masci merged commit 6d3f4ed into master May 22, 2020
@masci masci deleted the massi/publish branch May 22, 2020 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: documentation Related to documentation for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants