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

One giant PR #45

Merged
merged 1 commit into from
Jan 2, 2023
Merged

One giant PR #45

merged 1 commit into from
Jan 2, 2023

Conversation

amandakube
Copy link
Collaborator

Hi Jesse,

To make editing easier over break, I was building a draft version of the textbook on GithubPages and so people were sending PRs to me. This is all of the edits since the last PR I submitted to you. I fixed all merge conflicts I saw as I went and also put the site through a dead link checker and fixed all dead links but didn't do anything with making code look nicer. I am still hoping to get a chapter from Dan but not sure when that will happen and we need a frozen version of the textbook for class starting tomorrow! Can you merge this and the other PRs on here and create a new release that we freeze to give students and have another working version we can continue adding to?

Thanks so much!
Amanda

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

also:

* fixed "markdown" cells erroneously set to "code" and tagged exception-
  raising cells for jupyter book build
  * (also: removed empty cells and capitalized a sentence)

* corrected broken internal links

* added authorship attribution to DataFrames chapter

Co-authored-by: Jesse London <jesselondon@gmail.com>
@jesteria
Copy link
Member

jesteria commented Jan 2, 2023

Sounds good to me. Thanks!


FYI:

  • I removed the workflow_dispatch change from the PR – it sounds like this was useful in your fork but I don't believe we need it here. (This repo's builds are triggered by Releases; and, otherwise, we can always build the textbook locally. Though, again, glad you were able to use this feature; and, it looks like perhaps you'd intended to remove this line as well.)
  • I wasn't sure for myself about self-attribution within the textbook; but, I see that you've added attribution to my sub-chapter on Lists/Sequences (thanks!). And so I presume I may as well do the same for my chapter on DataFrames.
  • Two cells in the Assignments sub-chapter were incorrectly of type "code" rather than "markdown", and as such these threw exceptions and were rendered oddly in the textbook. I switched these to markdown.
  • I silenced build errors/warnings due to Notebook cells which purposefully raise exceptions, by assigning those cells the tag: raises-exception.
  • The build also complained about a few broken internal links which it simply stripped from the end result. (E.g., in Arrays Slicing: [Lists](./1/Lists.ipynb) was no longer correct; rather, it had to be something like [Lists](../../1/Lists).) These were corrected.

@amandakube
Copy link
Collaborator Author

Responses Inline:

  • I removed the workflow_dispatch change from the PR – it sounds like this was useful in your fork but I don't believe we need it here. (This repo's builds are triggered by Releases; and, otherwise, we can always build the textbook locally. Though, again, glad you were able to use this feature; and, it looks like perhaps you'd intended to remove this line as well.)

Oh sorry, I thought I removed that before submitting the pull request! Thank you for removing

  • I wasn't sure for myself about self-attribution within the textbook; but, I see that you've added attribution to my sub-chapter on Lists/Sequences (thanks!). And so I presume I may as well do the same for my chapter on DataFrames.

Yes, of course!

  • Two cells in the Assignments sub-chapter were incorrectly of type "code" rather than "markdown", and as such these threw exceptions and were rendered oddly in the textbook. I switched these to markdown.

Thank you!

  • I silenced build errors/warnings due to Notebook cells which purposefully raise exceptions, by assigning those cells the tag: raises-exception.

Great!

  • The build also complained about a few broken internal links which it simply stripped from the end result. (E.g., in Arrays Slicing: Lists was no longer correct; rather, it had to be something like Lists.) These were corrected.

Huh odd, I thought I fixed all of the broken links. Not sure what happened there

@jesteria jesteria added the content textbook content label Jan 2, 2023
@jesteria jesteria merged commit b5f9a50 into uchicago-dsi:master Jan 2, 2023
@amandakube
Copy link
Collaborator Author

How will the frozen copy and then any subsequent releases for work in progress work? What link can we give students for a frozen copy?

@amandakube amandakube deleted the draft branch January 2, 2023 23:06
@jesteria
Copy link
Member

jesteria commented Jan 2, 2023

OK: this is all published!

Going forward, and regardless of what's merged to the master branch, that main site should remain the place (for students) to see the textbook: ds1.datascience.uchicago.edu.

So long as no one creates a Release, the content there won't change.

Any changes to the built site can be reviewed locally (manage build [--all]). Beyond that, if there's need for a draft/pre-print/staging website, I imagine this could be accomplished with a fork, (much the way you did in your fork).

Hope that does the trick!

campbelle1 pushed a commit to campbelle1/EC_textbook-datascience-1 that referenced this pull request Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content textbook content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants