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

General feedback and suggestions #9

Closed
jimgar opened this issue Dec 27, 2023 · 2 comments · Fixed by #10
Closed

General feedback and suggestions #9

jimgar opened this issue Dec 27, 2023 · 2 comments · Fixed by #10

Comments

@jimgar
Copy link

jimgar commented Dec 27, 2023

Hey Maëlle,

Thank you for inviting me to give feedback on Mastodon.

This is such a great idea for a package. Having actual projects set up to practice with is super cool, and I can see this being a fantastic teaching tool and reference. I've done some interactive testing and have a bit of feedback for points where I think things could be clearer/different. Just bear in mind that overall, I think the steps work very well.

For the readme

  • Having {gert} in the solution examples is fine, but given that ohshitgit requires at least basic git knowledge and the tip()s all reference the git cli, is it worth nixing {gert} from the readme? Or just having it mentioned as an alternative to the git cli? For example I'm a basic git user and am here to improve my git, so I'd prefer to use the cli rather than another interface. Seeing {gert} in the example made me wonder if I would have to use it rather than the cli
  • On mac I found it bothersome to navigate to the created tempdir by using withr::local_tempdir() as shown in the readme example. I keep a convenient scratch directory called ~/crud and ended up using that instead. Maybe there's an easy way to get to the tempdir and I don't know it - if so, it might be worth stating?
  • VS Code users will only see the instructions when they open the created directory and launch an R console. Is this worth noting in the readme?
  • I stumbled across this page about git log while testing saperlipopette and I feel like it would actually be worth a mention, considering how much of this stuff is about checking the git "history". Just knowing about git log -p is super useful, for example.

I found the tip() for exo_split_changes() a bit confusing

• Examine Git staging area.
• `git add --patch` to add a first chunk ('?' to see the help, 's' to split, 'y' to stage the first
chunk, 'd' to discard the others)
• `git commit -m "First change"`
• `git add --patch` to add a second chunk ('s' to split, 'y' to stage the first chunk, 'n' once to
discard the other)
• `git commit -m "Second change"`
• `git add *` to add the last chunk
• `git commit -m "Third change"`
• Examine Git history.

Would it be possible to add indentation to emphasise the series of steps, something like

• Examine Git staging area.
• `git add --patch` to add a first chunk 
  • 's' to split
  • 'y' to stage the first chunk
  • 'd' to discard the others
  • Optionally, '?' to see the help
• `git commit -m "First change"`
• `git add --patch` to add a second chunk 
  • 's' to split
  • 'y' to stage the first chunk
  • 'n' once to discard the other
• `git commit -m "Second change"`
• `git add *` to add the last chunk
• `git commit -m "Third change"`
• Examine Git history.

Project already exists

If I attempt to create a project in a location where it already exists, this happens. Maybe there should be a check to see if exists first?

r$> saperlipopette::exo_one_small_change(".")
ℹ Follow along in ./one-small-change!
[1] "./one-small-change"

r$> saperlipopette::exo_one_small_change(".")
Error in gert::git_commit(message = message, author = default_author(),  : 
  No staged files to commit. Run git_add() to select files.

Contextless message

If in a directory with existing RProj and attempting to create a project, I see this. It looks like a truncated usethis message?

r$> saperlipopette::exo_one_small_change(".")
Do you want to create anyway?

1: Absolutely not
2: Yup
3: Negative

Create all projects

Finally, I thought a super simple wrapper function that creates the entire suite of projects would be very useful! I tested it in principle and this works

saperlipopette <- function(path) {
  saperlipopette::exo_committed_to_main(path)
  saperlipopette::exo_latest_message(path)
  saperlipopette::exo_one_small_change(path)
  saperlipopette::exo_split_changes(path)
  invisible()
}
@maelle
Copy link
Owner

maelle commented Jan 11, 2024

Thanks so much @jimgar!! Starting to look at those now. This is super valuable, I'm very thankful. Happy New Year!

@maelle
Copy link
Owner

maelle commented Jan 11, 2024

I implemented various commits but will wait for #10 to close this.

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 a pull request may close this issue.

2 participants