Skip to content

Hot007 issue307 update guide#460

Merged
maxim-belkin merged 7 commits intodatacarpentry:gh-pagesfrom
hot007:hot007-issue307-update-guide
Jun 9, 2020
Merged

Hot007 issue307 update guide#460
maxim-belkin merged 7 commits intodatacarpentry:gh-pagesfrom
hot007:hot007-issue307-update-guide

Conversation

@hot007
Copy link
Copy Markdown
Contributor

@hot007 hot007 commented May 8, 2020

Add content to Instructors Guide to Python Ecology lesson, particularly the final (SQL) section.
(part of instructor check-out).

hot007 added 5 commits April 30, 2020 16:21
Rename a few sections to match lesson outline, add comments to episode 7 & 8, start contributing content to episode 9.
Add solution to Q2.
Add solutions/comments for second set of questions.
@maxim-belkin
Copy link
Copy Markdown
Contributor

Hi @hot007! Thank you for the PR!

I like your changes but I'm curious as to why you change some headings, e.g., "05-merging-data" to "05-merging-data-with-pandas". In this particular example, episode title is "Combining DataFrames with Pandas" and the filename is 05-merging-data.md (which, I believe, was used for titles). I think it makes sense to use either episode titles OR names of the files. I suspect using abbreviated episode titles might be confusing.

Other than that -- it looks great! thank you!

@maxim-belkin maxim-belkin added type:enhancement Propose enhancement to the lesson status:in progress Contributor working on issue labels Jun 5, 2020
@hot007
Copy link
Copy Markdown
Contributor Author

hot007 commented Jun 6, 2020

Hi Maxim,
Ah, I was trying to get the title closer to the episode title (explicitly saying "with pandas" but definitely fine to reject that bit!
No worries!
-Claire

Comment thread _extras/guide.md Outdated
Comment thread _extras/guide.md
Comment thread _extras/guide.md Outdated
Comment thread _extras/guide.md
@maxim-belkin
Copy link
Copy Markdown
Contributor

maxim-belkin commented Jun 6, 2020

Sounds good. Please update the PR

Hi Maxim,
Ah, I was trying to get the title closer to the episode title (explicitly saying "with pandas" but definitely fine to reject that bit!
No worries!
-Claire

Excellent! Could you please update the PR accordingly then? I think these headings can be episode titles, file names (without .md), or "Episode X: Episode title". Using episode titles make the most sense to me but I don't mind if you keep them as they were.

Thank you for your work and let me know if you need help or advice.

Maxim

Thanks for the review @maxim-belkin changes made as requested, hope I got all of them. Thanks for the {: .output} tip, I didn't know about that.
@hot007
Copy link
Copy Markdown
Contributor Author

hot007 commented Jun 9, 2020

Okay, I've made the requested changes I hope, please let me know if I missed anything. Thanks for the feedback, I'm learning about this {: .} syntax, that's really helpful, apologies I had it wrong, the preview for me isn't parsing those bits so I couldn't see it wasn't working.
I haven't changed the other section headings though, do we really want to call one of the sections "07-visualization-ggplot-python" when ggplot has been replaced with plotnine? Or should we rename that episode file? Similarly for Eps 6, 8 and 9, whose filenames are now substantially out of line with the episode titles. Actually I will make another commit rolling back my changes to the headings, but just noting the filenames probably should be changed so that changes like this can be made in the instructor's guide.

@maxim-belkin
Copy link
Copy Markdown
Contributor

I haven't changed the other section headings though, do we really want to call one of the sections "07-visualization-ggplot-python" when ggplot has been replaced with plotnine? Or should we rename that episode file? Similarly for Eps 6, 8 and 9, whose filenames are now substantially out of line with the episode titles. Actually I will make another commit rolling back my changes to the headings, but just noting the filenames probably should be changed so that changes like this can be made in the instructor's guide.

My personal preference is to use "Ep. #: Episode Title" headings but what you did here makes perfect sense. Of course, we should keep episode file names in sync with the content, e.g. ggplot -> plotnine, etc., but these discrepancies should be addressed in separate PRs (1 file change per PR).

I'm going to merge this PR as it seems to be ready. Thank you for the excellent contribution! If you'd like to work on any of the open issues -- let me know and I'll be happy to help!

🏅

@maxim-belkin maxim-belkin merged commit 5c40834 into datacarpentry:gh-pages Jun 9, 2020
@maxim-belkin maxim-belkin removed the status:in progress Contributor working on issue label Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:enhancement Propose enhancement to the lesson

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants