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

Add a "no quizzes" handling ability as well as some merge conflict resolutions #480

Merged
merged 29 commits into from
Feb 19, 2022

Conversation

cansavvy
Copy link
Collaborator

@cansavvy cansavvy commented Feb 16, 2022

Purpose/implementation Section

What changes are being implemented in this Pull Request?

Addresses #478 which involves allowing render-all to run but skip handling quizzes.

This is needed for folks who never want quizzes, or if they want their quizzes to be hidden aka they will use a _Quizzes repository.

There are other little bugs and things that I found needed polishing. See the comments I made for explaining each add.

I've been testing these on https://github.com/jhudsl/Adv_Reproducibility_in_Cancer_Informatics/ and https://github.com/jhudsl/Reproducibility_in_Cancer_Informatics/ by sending the changes individually to those repos.

If we think this is reasonable, I think we should send out a sync with these changes right away because it does solve a lot of those merge conflict problems that we inconsistently encounter on various repos.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2022

No spelling errors! 🎉
Comment updated at 2022-02-18 with changes from ea83b43

@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2022

Re-rendered previews from the latest commit:

Updated at 2022-02-18 with changes from ea83b43

@cansavvy
Copy link
Collaborator Author

This will not work before ottrpal has the ability to skip quizzes too, which I will work on.

@cansavvy cansavvy changed the title There's no quizzes option for rendering Leanpub/Coursera Add a "no quizzes" option for rendering Leanpub/Coursera Feb 17, 2022
run: |
git config --local user.email "actions@github.com"
git config --local user.name "GitHub Actions"
git remote set-url origin https://${GH_PAT}@github.com/${GITHUB_REPOSITORY}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This gives the render permission to push straight to main

@@ -138,6 +143,10 @@ jobs:
# Create screenshots
- name: Run the screenshot creation
run: |
# Remove old folder
rm -rf resources/chapt_screen_images
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ensures a refresh of the images each time so no old files hang around

@@ -147,6 +156,16 @@ jobs:
run: rm -rf manuscript/

- name: Run ottrpal::bookdown_to_embed_leanpub
if: needs.yaml-check.outputs.toggle_quiz_check == 'no'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If quizzes aren't being checked here, we can set quiz_dir = NULL and assume there are not quizzes here.

@@ -184,15 +205,17 @@ jobs:

# Run Coursera version
- name: Convert Leanpub quizzes to Coursera
if: needs.yaml-check.outputs.toggle_leanpub == 'yes'
if: needs.yaml-check.outputs.toggle_leanpub == 'yes' && needs.yaml-check.outputs.toggle_quiz_check == 'yes'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This step for coursera is really only needed if there's quizzes AND if Leanpub is being published.

runs-on: ubuntu-latest
file-quizzes-pr:
name: File _Quizzes Transfer PR
needs: [yaml-check]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This wasn't running properly because I forgot to specify it needed yaml-check

config_automation.yml Outdated Show resolved Hide resolved
@@ -54,7 +54,7 @@ if (is.null(opt$base_url)) {
base_url <- cow::get_pages_url(repo_name = opt$repo, git_pat = opt$git_pat)
}

chapt_df <- ottrpal::get_chapters(base_url = file.path(base_url, "no_toc"))
chapt_df <- ottrpal::get_chapters(base_url = file.path(base_url, "no_toc/"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This / is needed or else the image names get messed up.

@cansavvy cansavvy requested a review from avahoffman February 18, 2022 14:13
@cansavvy cansavvy changed the title Add a "no quizzes" option for rendering Leanpub/Coursera Add a "no quizzes" handling ability as well as some merge conflict resolutions Feb 18, 2022
Copy link
Member

@carriewright11 carriewright11 left a comment

Choose a reason for hiding this comment

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

It all looks good! but we are getting rid of the template quizzes?

@cansavvy
Copy link
Collaborator Author

It all looks good! but we are getting rid of the template quizzes?

Ah, that was to test that it would work without quizzes, but I will restore them now.

@carriewright11
Copy link
Member

It all looks good! but we are getting rid of the template quizzes?

Ah, that was to test that it would work without quizzes, but I will restore them now.

sounds good!

@carriewright11
Copy link
Member

Also when we go with "their" history - is that the main branch history?

@cansavvy
Copy link
Collaborator Author

@carriewright11

Also when we go with "their" history - is that the main branch history?

Yes. Honestly the main issues come up with binary files like .docx and .rds files.
https://howchoo.com/git/git-merge-conflicts-rebase-ours-theirs

Now I'm wondering if we should default on keeping the current branch's though so ours. What do you think? For the preview it's less consequential than for render-all.yml but whatever it is we want it to be the same in both so the preview is truly representative.

@carriewright11
Copy link
Member

@carriewright11

Also when we go with "their" history - is that the main branch history?

Yes. Honestly the main issues come up with binary files like .docx and .rds files. https://howchoo.com/git/git-merge-conflicts-rebase-ours-theirs

Now I'm wondering if we should default on keeping the current branch's though so ours. What do you think? For the preview it's less consequential than for render-all.yml but whatever it is we want it to be the same in both so the preview is truly representative.

hmmm good question. I think I agree that ours might be the better choice

@cansavvy
Copy link
Collaborator Author

hmmm good question. I think I agree that ours might be the better choice

Yes, I think I agree. Will switch

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.

2 participants