-
Notifications
You must be signed in to change notification settings - Fork 77
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
Rename template folder and files in preparation of migration to copier #411
Conversation
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
@@ -6,15 +6,15 @@ package_name: | |||
help: Enter the name of the Python package. | |||
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_name
is not being asked anymore becausecopier copy source destination
generates it. It was replaced by eithertemplate
to refer to the location orpackage_name
to construct the github links. This means thatpackage_name
is now also the name of the repo.Related issues: