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

Renaming github_username to github_owner + generating __repo_name & __repo_url #409

Merged
merged 11 commits into from
Nov 5, 2024

Conversation

matt-graham
Copy link
Collaborator

Though its not exposed to user now we have longer prompts rather than using variable names directly, I think github_username is a bit of a misnomer given that it can be either a user or organization name, hence I think github_owner would be a better choice.

We also currently manually construct the GitHub repository URL in lots of different bits of the template and also the qualified repository name in a few places. We can use double underscore prefixed (rendered) private variables in the cookiecutter config to generate the repository URL and qualified name once and then reuse elsewhere to avoid the repetition. This would also make it simpler to later switch to supporting alternative repository hosting options such as GitLab in future (which is the rational for naming the variables __repo_name and __repo_url rather than __github_repo_name and __github_repo_url).

@matt-graham matt-graham added the enhancement New feature or request label Jun 7, 2024
@dstansby
Copy link
Member

dstansby commented Jun 7, 2024

I'm 👍 , but reluctant to approve before we have some regression tests (#329) since there's a lot of changes here that could be susceptible to human error (e.g., missing or adding "/" characters from URLs)

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.

Oo I didn't know about the private variables, big fan of this approach.

Are there any other private variables that we could leverage?

@samcunliffe
Copy link
Member

I'm 👍 , but reluctant to approve before we have some regression tests (#329) since there's a lot of changes here that could be susceptible to human error (e.g., missing or adding "/" characters from URLs)

Isn't it enough the tests we currently have, all pass? We do do a cookiecut operation in CI, right? Which is working...

def test_package_generation(

Copy link
Member

@samcunliffe samcunliffe left a comment

Choose a reason for hiding this comment

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

🫐

(Conflicts, obvs)

@paddyroddy paddyroddy added the needs-2-reviewers Considered "controversial" so worth a second pair of eyes label Oct 22, 2024
@matt-graham
Copy link
Collaborator Author

Tests were previously failing since #432 was merged in as the passed configs now need to use github_owner rather than github_username - now fixed and have also updated tutorial to make more consistent with new github_owner variable name.

Copy link
Member

@samcunliffe samcunliffe left a comment

Choose a reason for hiding this comment

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

🫐

@samcunliffe
Copy link
Member

Let's make v1.0.0 of the template when this is in.

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.

🫐

"min_python_version": ["3.11", "3.12", "3.13"],
"max_python_version": ["3.13", "3.12", "3.11"],
"license": ["MIT", "BSD-3", "GPL-3.0"],
"funder": "",
"__repo_name": "{{cookiecutter.github_owner}}/{{cookiecutter.project_slug}}",
Copy link
Member

Choose a reason for hiding this comment

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

TIL __variable in cookiecutter.json, looks so clean!

@matt-graham matt-graham merged commit 85b0b29 into main Nov 5, 2024
13 checks passed
@matt-graham matt-graham deleted the mmg/rename-to-github-owner branch November 5, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants