-
Notifications
You must be signed in to change notification settings - Fork 84
Rename template folder and files in preparation of migration to copier #411
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
Conversation
sjvrijn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just two small remarks. Tests pass for me locally conditional on #408 👍
copier.yml
Outdated
| validator: >- | ||
| {% if not (package_name | regex_search('^[a-z][a-z0-9\_]+$')) %} | ||
| package_name must start with a letter, followed one or more letters, digits or underscores all lowercase. | ||
| package_name must start with a letter, followed one or more letters, digits or underscores all lowercase. Avoid using spaces or uppercase letters for the best experience across operating systems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to make this consistent with the README by also specifying to avoid dashes, i.e., 'Avoid using spaces, dashes or uppercase letters [...]'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came from the README :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a restriction, as user is not allowed to use any of that. Shall we get rid of it altogher?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The directory_name explanation indeed did not mention dashes, I was looking at the explanation for package_name, which does suggest avoiding them. Either way, I agree that removing it even better since it's not allowed anyway 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review @sjvrijn. I have made changes and updated branch. This can be merged now.
c6cc0e0 to
7c89fe9
Compare
Description
In copier
directory_nameis not being asked anymore becausecopier copy source destinationgenerates it. It was replaced by eithertemplateto refer to the location orpackage_nameto construct the github links. This means thatpackage_nameis now also the name of the repo.Related issues: