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

Rework README and move dev instructions to contributing #481

Merged

Conversation

samcunliffe
Copy link
Member

I've attempted to simplify the user-facing quick-start instructions. Not a huge fan of enumerated instructions and the optional (but cool) one-liner it doesn't really fit in 1. 2. 3....

Also it's a bit overkill to have a venv/conda environment just for cookiecutter itself. The cookiecutter docs recommend pipx install. I claim that we want to encourage uvx because it's fast and cool 😎 .

I've spent a bit of effort trying to be clear but comments on clarity etc are very welcome.

I also moved and merged the dev instructions into CONTRIBUTING so README is just user-facing. And CONTRIBUTING is just dev.

This addresses

And two comments

Move all developer instructions out from README and rework/update the instructions.
Disable markdownlint rule MD033 globally.
@samcunliffe samcunliffe self-assigned this Oct 29, 2024
@samcunliffe samcunliffe added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 29, 2024
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@samcunliffe samcunliffe added the needs-2-reviewers Considered "controversial" so worth a second pair of eyes label Oct 29, 2024
Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

I like the idea but see my concerns on DRYness

CONTRIBUTING.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Looking good 👍 - I've left some nitpicks that you can take or leave

.markdownlint.yaml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
```

You can also [install cookiecutter].
Do this if you don't use [uv], or if you're likely to want to use cookiecutter again.
Copy link
Member

Choose a reason for hiding this comment

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

This says "you can do this if you don't use uv", but then the command below uses uv. Perhaps just link to https://cookiecutter.readthedocs.io/en/stable/README.html#installation instead of offering extra commands?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do link to there. Line above ☝️
And I thought "Here's the way we install it" indicated that there are other ways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could change

[install cookiecutter]

to

[install cookiecutter] with pipx or pip

to make it clearer that this is distinct from the uv installation instructions and maybe also change

Here's the way we install it:

to

To install cookiecutter using uv run

Copy link
Member Author

Choose a reason for hiding this comment

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

How about just removing the whole thing and directing the user to install cookiecutter following cookiecutter's instructions?

6816c2e

Then it becomes a bit simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good to me!

README.md Outdated Show resolved Hide resolved
@samcunliffe
Copy link
Member Author

pre-commit.ci autofix

Copy link
Collaborator

@matt-graham matt-graham left a comment

Choose a reason for hiding this comment

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

Looks much clearer to me overall, thanks @samcunliffe! I agree with @paddyroddy's suggestions to remove example run through of cookiecutter and resulting project directory tree as seem like these will just be a pain to keep up to date without adding much value for users. Have also added some suggested changes around @dstansby's comments on installing cookiecutter.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
```

You can also [install cookiecutter].
Do this if you don't use [uv], or if you're likely to want to use cookiecutter again.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could change

[install cookiecutter]

to

[install cookiecutter] with pipx or pip

to make it clearer that this is distinct from the uv installation instructions and maybe also change

Here's the way we install it:

to

To install cookiecutter using uv run

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.markdownlint.yaml Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@@ -1,2 +1,4 @@
---
MD013: false
# Markdown linting rules to ignore.
Copy link
Member

Choose a reason for hiding this comment

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

Feels like you've done this to trigger me...

Given that you can do other things besides ignoring in this file. I think we should remove this comment.

Suggested change
# Markdown linting rules to ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

But everything in here is a rule to ignore.

you can do other things

We're not doing other things.

Copy link
Member

Choose a reason for hiding this comment

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

But we might?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we would add another comment above the block of other things?

Copy link
Member

Choose a reason for hiding this comment

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

Feels unnecessary, but okay

@paddyroddy paddyroddy self-requested a review November 4, 2024 15:12
@samcunliffe samcunliffe merged commit d5b41c7 into main Nov 4, 2024
13 checks passed
@samcunliffe samcunliffe deleted the sc/rework-readme-and-move-dev-instructions-to-contributing branch November 4, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request needs-2-reviewers Considered "controversial" so worth a second pair of eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants