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 instructor notes 505 #509

Merged
merged 9 commits into from
Aug 31, 2023
Merged

Fix instructor notes 505 #509

merged 9 commits into from
Aug 31, 2023

Conversation

klbarnes20
Copy link
Contributor

@klbarnes20 klbarnes20 commented Aug 30, 2023

This PR attempts to fix #505. It adds a test for when instructor notes is empty and an else clause to the build_instructor_notes.R.

When I ran the test before the fix, it failed. When I ran it after, that particular test passed. I still have two other failed tests, but not sure they are related to this issue/fix.

@zkamvar
Copy link
Contributor

zkamvar commented Aug 30, 2023

Thank you for submitting the fix, Kelly!

When I ran the test before the fix, it failed. When I ran it after, that particular test passed. I still have two other failed tests, but not sure they are related to this issue/fix.

The other two failed tests have to do with the fact that 533d019 accidentally deleted a snapshot test (probably because it was skipped on windows). Now that pandoc is more reliably set up for windows, it may be worthwhile revisiting the tests that skip on windows for pandoc, but for now, i will reinstate that snapshot

Copy link
Contributor

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

Thank you, Kelly. I have one suggestion that will likely fix the failing test.

tests/testthat/test-build_lesson.R Outdated Show resolved Hide resolved
@klbarnes20
Copy link
Contributor Author

Thanks @zkamvar! I've committed your suggestions. If the tests pass and you think its ready, feel free to merge.

@zkamvar zkamvar merged commit e283e28 into main Aug 31, 2023
12 checks passed
@zkamvar zkamvar deleted the fix_instructor_notes_505 branch August 31, 2023 16:57
@zkamvar
Copy link
Contributor

zkamvar commented Aug 31, 2023

Congratulations, @klbarnes20 you have your first contribution to sandpaper!

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.

Bug: XML Parsing error if instructor notes are empty
2 participants