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

Migrated to copier #34

Merged
merged 9 commits into from
Aug 1, 2024
Merged

Migrated to copier #34

merged 9 commits into from
Aug 1, 2024

Conversation

santacodes
Copy link
Collaborator

@santacodes santacodes commented Jul 30, 2024

This is a follow-up on PR #29, the template has been completely migrated to copier as copier is under active development and is better maintained than cookiecutter.

While migrating to copier I faced some issues and thought I would mention them here, firstly in the copier.yml (default configuration file) we cannot add our variables or constants and set their values like we did for cookiecutter for __project_slug and __year. To tackle this problem, I added a prompt for project_slug and replaced the __year instances with time labels derived from jinja2_time.TimeExtension.
As of now the generated project using copier works well and the tests within the generated project are all set, though I have yet to add the template generation tests. For tests I would be using pytest-copie linked here which is almost the same as pytest-cookies that we are currently using.

Usage

To generate a project in a directory called test/ you would need to perform -

copier copy pybamm-cookiecutter/ test/   --trust # This will generate the src layout of the project inside the test directory
# copier prompts the user for details (the --trust flag is to bypass a warning caused by the `jinja2_time.TimeExtension`)

Additional Notes

I will be working on these template generation test cases with all the build-backends and also the cli. The CLI would wrap everything and just a single command would set up the working directory with project files.

Sub-task #26

@agriyakhetarpal
Copy link
Member

Thanks for taking the initiative to put up this PR early! I think it will be easier to review once we merge #29 so that the diff gets cleaner (and once you add the template generation tests using pytest-copie, as you mentioned).

@santacodes santacodes reopened this Jul 31, 2024
copier.yml Outdated Show resolved Hide resolved
copier.yml Outdated Show resolved Hide resolved
copier.yml Outdated Show resolved Hide resolved
copier.yml Outdated Show resolved Hide resolved
copier.yml Outdated Show resolved Hide resolved
template/pyproject.toml.jinja Outdated Show resolved Hide resolved
template/pyproject.toml.jinja Outdated Show resolved Hide resolved
template/pyproject.toml.jinja Outdated Show resolved Hide resolved
template/src/{{project_slug}}/__init__.py.jinja Outdated Show resolved Hide resolved
@santacodes santacodes force-pushed the copier branch 2 times, most recently from 315a5ee to 6ed4eb8 Compare July 31, 2024 18:35
Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Thanks, @santacodes! Could you please fix the conflicts? Happy to approve once the conflicts and the previous conversations are addressed.

.github/workflows/test_on_push.yml Outdated Show resolved Hide resolved
copier.yml Outdated Show resolved Hide resolved
copier.yml Outdated Show resolved Hide resolved
copier.yml Outdated Show resolved Hide resolved
copier.yml Outdated Show resolved Hide resolved
template/pyproject.toml.jinja Outdated Show resolved Hide resolved
template/pyproject.toml.jinja Outdated Show resolved Hide resolved
template/pyproject.toml.jinja Outdated Show resolved Hide resolved
template/pyproject.toml.jinja Outdated Show resolved Hide resolved
@agriyakhetarpal agriyakhetarpal self-requested a review August 1, 2024 13:26
Copy link
Sponsor Member

@brosaplanella brosaplanella left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Looks great now, thanks, @santacodes!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
Co-authored-by: Agriya Khetarpal <[email protected]>
Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Looks good @santacodes, this one was fast 🚀
You've tested this locally too, right?

@santacodes
Copy link
Collaborator Author

Looks good @santacodes, this one was fast 🚀
You've tested this locally too, right?

Yes I did test it locally.

@santacodes santacodes merged commit 8ec9f4c into pybamm-team:main Aug 1, 2024
35 checks passed
@santacodes santacodes deleted the copier branch August 30, 2024 12:39
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 this pull request may close these issues.

4 participants