Apply latest Copier template updates#247
Conversation
AndrewSazonov
added
the
[scope] maintenance
henrikjacobsenfys
left a comment
henrikjacobsenfys
left a comment
There was a problem hiding this comment.
I looked at a handful of randomly selected files, and they looked fine.
There was a problem hiding this comment.
I spent way too much time on this PR :)
Rechecked actions version bump correctness,
Assured client-id is the new version of app-ad in create-github-app-token action
Checked dependency move in pyproject.toml
Got worried about manually copying trstringer/require-label-prefix action
Very useful addition of nodefaults to pixi channels
etc.
One potential issue I have is - do we really, REALLY, need to add the useless docstrings to all the methods?
def __init__(self):
"""Init function."""def __call__(self, *args, **kwargs):
"""Call function."""Instead of adding junk, why not just # noqa: DOC comment for those trivial dunder methods?
Yeah, I am not a big fan of that either. But on the other hand, that action is currently not maintained. |
I believe this was required for us now, right? To make 100% sure we don't accidentally get any commercial conda-packages. |
| unique_name : Optional[str], default=None | ||
| Optional unique name for the list. By default, None. | ||
| display_name : Optional[str], default=None | ||
| Optional display name for the list. By default, None. |
There was a problem hiding this comment.
The automatic docstring conversion clearly took some liberties in regards to scanning scanning the code and re-writing the docstrings. We probably need to manually go through the docstrings after this and ammend them.
There was a problem hiding this comment.
I agree that a manual check is needed, and that we should all go through the docstrings carefully. But do you suggest doing this as part of the current PR, or plan it as a follow-up task during future development?
There was a problem hiding this comment.
Definitely a follow-up task for a future documentation effort :)
|
|
||
| [tool.ruff.lint.flake8-tidy-imports] | ||
| # Disallow all relative imports | ||
| ban-relative-imports = 'all' |
There was a problem hiding this comment.
Didn't we discuss that this was a developer preference? I for one prefer relative imports over absolute imports.
There was a problem hiding this comment.
Right, we did discuss this. Personally, I do not mind whether imports are absolute or relative, but I do care about consistency.
It seems that the only way to guarantee consistency is to enforce absolute imports everywhere. Otherwise, we end up with a random mix of both styles in different places.
Therefore, I prefer absolute imports. That said, if the majority prefers relative imports, I am okay with removing this setting.
There was a problem hiding this comment.
Or maybe add this as a question at the template generation stage? Default to relative but allow override.
Would that be a solution?
There was a problem hiding this comment.
This can of course be added and configured differently for different technique-specific projects. But I think the main question here is which style we want to use in the easyscience module.
There was a problem hiding this comment.
Added feature request: easyscience/templates#35
|
The one blocking issue is the absolute path fix in |
Agreed, these placeholder docstrings are not useful. I also prefer not to add So I removed the useless placeholder docstrings instead. |
Thanks for finding that. I switched the templates from the custom script to |
This issue is resolved now. Thanks for suggesting the fix. |
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
Yeah, I think this is ready to be merged now.
4 participants
This PR syncs the project with the latest Copier templates. Main changes include replacing deprecated Node.js 20 usage, migrating docstrings from Google style to NumPy style, and applying related template-driven configuration updates.