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

Recipes draft #1003

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Recipes draft #1003

wants to merge 17 commits into from

Conversation

martinosorb
Copy link
Contributor

@martinosorb martinosorb commented Jul 3, 2024

I created this PR to check if it was possible to merge the Recipes version without too much pain, and so we can discuss whether this is a good choice.
Before merging we have a number of TODOs:

  • Ask @dpshelio if there are any objections to using their material and how we can acknowledge them
  • Ask @swcarpentry/git-novice-es-maintainers how much would this affect the Spanish language fork and whether they are ok with updating their translation accordingly
  • As @swcarpentry/git-novice-maintainers, consider if this is what we want to do, as opposed to other choices
  • Under suggestion from @kekoziar, avoid cocktails and use something alcohol-free instead
  • Gather opinions from the CAC @swcarpentry/governance-committee

dpshelio and others added 3 commits July 13, 2023 20:17
Copy link

github-actions bot commented Jul 3, 2024

Thank you!

Thank you for your pull request 😃

🤖 This automated message can help you check the rendered files in your submission for clarity. If you have any questions, please feel free to open an issue in {sandpaper}.

If you have files that automatically render output (e.g. R Markdown), then you should check for the following:

  • 🎯 correct output
  • 🖼️ correct figures
  • ❓ new warnings
  • ‼️ new errors

Rendered Changes

🔍 Inspect the changes: https://github.com/swcarpentry/git-novice/compare/md-outputs..md-outputs-PR-1003

The following changes were observed in the rendered markdown documents:

 02-setup.md                          |  12 +-
 03-create.md                         |  75 +++++------
 04-changes.md                        | 246 +++++++++++++++++++---------------
 05-history.md                        | 224 ++++++++++++++++---------------
 06-ignore.md                         | 100 +++++++-------
 07-github.md                         |  85 ++++++------
 08-collab.md                         |  45 ++++---
 09-conflict.md                       | 250 +++++++++++++++++++++--------------
 14-supplemental-rstudio.md           |   8 +-
 discuss.md                           | 130 +++++++++---------
 fig/git-freshly-made-github-repo.svg |   4 +-
 fig/github-collaboration.svg         |  10 +-
 fig/github-repo-after-first-push.svg |   6 +-
 index.md                             |   8 +-
 instructor-notes.md                  |   8 +-
 md5sum.txt                           |  24 ++--
 16 files changed, 666 insertions(+), 569 deletions(-)
What does this mean?

If you have source files that require output and figures to be generated (e.g. R Markdown), then it is important to make sure the generated figures and output are reproducible.

This output provides a way for you to inspect the output in a diff-friendly manner so that it's easy to see the changes that occur due to new software versions or randomisation.

⏱️ Updated at 2024-09-18 12:43:15 +0000

github-actions bot pushed a commit that referenced this pull request Jul 3, 2024
@martinosorb martinosorb added the type:discussion Discussion or feedback about the lesson label Jul 3, 2024
episodes/07-github.md Outdated Show resolved Hide resolved
episodes/09-conflict.md Outdated Show resolved Hide resolved
episodes/09-conflict.md Outdated Show resolved Hide resolved
episodes/09-conflict.md Outdated Show resolved Hide resolved
index.md Outdated Show resolved Hide resolved
learners/discuss.md Outdated Show resolved Hide resolved
episodes/05-history.md Outdated Show resolved Hide resolved
learners/discuss.md Outdated Show resolved Hide resolved
learners/discuss.md Outdated Show resolved Hide resolved
learners/discuss.md Outdated Show resolved Hide resolved
learners/discuss.md Outdated Show resolved Hide resolved
@martinosorb
Copy link
Contributor Author

@nsoranzo thank you so much for your suggestions. After searching around for established conventions, I agree both with the idea of capitalizing Git and with using present tense rather than present continuous in commit messages. However, these are changes that are unrelated to this pull request. Any reason why you wanted to suggest this here? It would be best to create another PR after this one has been merged (if it's approved). Thank you though.

@nsoranzo
Copy link
Contributor

nsoranzo commented Jul 4, 2024

@nsoranzo thank you so much for your suggestions. After searching around for established conventions, I agree both with the idea of capitalizing Git and with using present tense rather than present continuous in commit messages. However, these are changes that are unrelated to this pull request. Any reason why you wanted to suggest this here? It would be best to create another PR after this one has been merged (if it's approved). Thank you though.

Thanks for looking into my suggestions! I partially agree with your reply:

  • For capitalising Git, this is already the case in most of the episodes and my suggestions where only for a new callout, so I cannot do that in a separate PR. Actually I wanted to suggest that the new callout should go in a separate PR 😄
  • For the use of imperative rather than the -ing form, I can do that for the current version of the lesson, but then it would conflict with your changes here, let me know if that's your preferred choice.

github-actions bot pushed a commit that referenced this pull request Jul 5, 2024
github-actions bot pushed a commit that referenced this pull request Jul 5, 2024
Copy link
Member

@tobyhodges tobyhodges left a comment

Choose a reason for hiding this comment

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

On the whole, this looks good and results in less of a change (in practical terms) than I had expected. I have suggested a minor change to the Markdown syntax used in the examples: having multiple h1 headings in a file is bad practice from an accessibility standpoint, so I have suggested h2 headings (##) instead.

A few other miscellaneous questions and suggestions too.

episodes/03-create.md Outdated Show resolved Hide resolved
episodes/04-changes.md Outdated Show resolved Hide resolved
episodes/04-changes.md Outdated Show resolved Hide resolved
episodes/04-changes.md Outdated Show resolved Hide resolved
episodes/04-changes.md Outdated Show resolved Hide resolved
episodes/09-conflict.md Outdated Show resolved Hide resolved
episodes/09-conflict.md Outdated Show resolved Hide resolved
episodes/09-conflict.md Outdated Show resolved Hide resolved
learners/discuss.md Show resolved Hide resolved
learners/discuss.md Show resolved Hide resolved
martinosorb and others added 3 commits August 5, 2024 10:57
Co-authored-by: Toby Hodges <tobyhodges@carpentries.org>
Markdown accepts either `-` or `*` however, in this material, the `-`
causes confussion when looking at diffs as you'd get things like:

```diff
 - avocado
-- lemon
+- lime
```

making the `-` from the diff misunderstood with the `-` of the list.
This is done to have files with a single heading as accessibility
recommendations. A title is introduced (recipe title) and the
ingredients and instructions are set as 2nd level headings.
@tobyhodges
Copy link
Member

If and when #1013 is merged, I will be happy to approve this.

@dpshelio
Copy link

dpshelio commented Aug 14, 2024

Ups! I didn't see your comment, @tobyhodges - I merged also #1012 into #1013 because it was producing a lot of merge conflicts. Hopefully that doesn't change your mind Oh! I saw it doesn't 😇

martinosorb and others added 4 commits August 14, 2024 18:24
Co-authored-by: Nicola Soranzo <nicola.soranzo@earlham.ac.uk>
Co-authored-by: Nicola Soranzo <nicola.soranzo@earlham.ac.uk>
Co-authored-by: Toby Hodges <tobyhodges@carpentries.org>
Co-authored-by: Toby Hodges <tobyhodges@carpentries.org>
@martinosorb
Copy link
Contributor Author

I've incorporated the suggestions by @dpshelio, @nsoranzo, and @tobyhodges. The only conversation left is about the sentence "A supplemental episode to this lesson discusses advanced setup and concepts of SSH and key pairs, and other material supplemental to git related SSH" that was introduced, and probably refers to something internal to UCL, I'm guessing. I will now proceed and merge #1013. After that, we can all set a date for merging. At that point, I will rebase and fix the conflicts.

Modifies examples to include a single heading and changes md symbol used for listing items
@dpshelio
Copy link

Nothing to do with UCL 😇 - that was introduced in 2021 by @kekoziar
2b4bdb0#diff-6630f5f1e305bfd380d35f1f94de9c6c1c8f706ee2671c875f05db70b42f2dacR88-R90
and removed last year
d01e39b

@MGomezN
Copy link

MGomezN commented Aug 14, 2024

Hello! I'm a git-novice-es-maintainer
I missed/forgot this 👇🏽 , but I'm happy to help update the Spanish version once this PR is approved.

Ask @swcarpentry/git-novice-es-maintainers how much would this affect the Spanish language fork and whether they are ok with updating their translation accordingly

@martinosorb martinosorb marked this pull request as ready for review August 15, 2024 14:34
github-actions bot pushed a commit that referenced this pull request Aug 15, 2024
@martinosorb martinosorb removed the type:discussion Discussion or feedback about the lesson label Aug 15, 2024
github-actions bot pushed a commit that referenced this pull request Aug 15, 2024
@jas58
Copy link

jas58 commented Aug 21, 2024

This is so cool! Thank you for the the revisions and work!

@chennesy
Copy link

These look like great improvements, many thanks to all who worked on them!
@kekoziar @martinosorb: is there a target date for approving the PR? I have instructors planning to teach this on Sept 26, and want to make sure we know which version will be live since these are pretty significant changes.

@jas58
Copy link

jas58 commented Sep 13, 2024 via email

@martinosorb
Copy link
Contributor Author

Paging @tobyhodges on timing. If you guys would like to have this version for your workshop, I think we could arrange to have it merged before then.

@chennesy
Copy link

Thanks @martinosorb @tobyhodges Our instructors noted they won't have much time to review the changes in their prep before their workshop so are wondering if it would be OK to wait until after Sept 26, 2024 1800 UTC to merge the PR?

@martinosorb
Copy link
Contributor Author

@jas58 @chennesy, we will then merge between the 27th and 30th of September. Thanks.

learners/discuss.md Outdated Show resolved Hide resolved
Co-authored-by: James Foster <38274066+jd-foster@users.noreply.github.com>
@martinosorb
Copy link
Contributor Author

Thanks for catching that typo @jd-foster.

github-actions bot pushed a commit that referenced this pull request Sep 18, 2024
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.

8 participants