feat: basic structure based on cookiecutter #3
Conversation
Signed-off-by: Prati28 <[email protected]>
Reviewer's Guide by SourceryThis pull request sets up the basic project structure using Cookiecutter. It includes the addition of Makefile, Sphinx documentation configuration, GitHub Actions workflows for CI/CD, community guidelines, configuration files for safety checks and pre-commit hooks, placeholder test files, and the main entry point for the tus_storagehandler package. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @psankhe28 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟡 Documentation: 2 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| run: | | ||
| SEP="${{ !startsWith(runner.os, 'windows') && '/' || '\\' }}" | ||
| PIPX_CACHE="${{ github.workspace }}${SEP}pipx_cache" | ||
| echo "pipx-cache-path=${PIPX_CACHE}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
nitpick: Use consistent quoting style
Consider using consistent quoting style for variables and strings. Mixing single and double quotes can lead to readability issues and potential bugs.
| - name: Set up environment | ||
| uses: ./.github/actions/setup/poetry | ||
| with: | ||
| os: ${{ job.os }} |
There was a problem hiding this comment.
suggestion: Remove unnecessary input
The os input is not used in the setup/poetry action. Consider removing it to avoid confusion.
| os: ${{ job.os }} | |
| python-version: '3.11' | |
| poetry-install-options: "--only=lint --no-root" |
| poetry run pytest \ | ||
| --cov-report term \ | ||
| --cov-report xml:test_integration.xml \ | ||
| --cov=tests/test_integration |
There was a problem hiding this comment.
suggestion: Use consistent coverage directory
Consider using a consistent directory for coverage reports across different test jobs. This will make it easier to manage and compare coverage results.
| --cov=tests/test_integration | |
| --cov=coverage/tests/test_integration |
| run: | | ||
| poetry lock --no-update | ||
| poetry install | ||
| pre-commit run --all-files |
There was a problem hiding this comment.
suggestion: Add specific pre-commit stage
Consider specifying the pre-commit stage (e.g., commit, push) to ensure that the appropriate hooks are run. This can help avoid running unnecessary hooks.
| pre-commit run --all-files | |
| pre-commit run --all-files --hook-stage commit |
| - name: Compare docs with main branch | ||
| id: check_docs_diff | ||
| run: | | ||
| shasum /tmp/docs/* > /tmp/docs.sha |
There was a problem hiding this comment.
issue: Handle empty directories
Ensure that the shasum command handles empty directories gracefully. Consider adding a check to avoid errors when no files are present.
|
|
||
| def main(): | ||
| """Main entry point for tus_storagehandler.""" | ||
| print("Hello from tus_storagehandler!") |
There was a problem hiding this comment.
suggestion: Avoid using print statements
Consider using a logging framework instead of print statements for better control over log levels and output destinations.
| print("Hello from tus_storagehandler!") | |
| import logging | |
| def main(): | |
| """Main entry point for tus_storagehandler.""" | |
| logging.basicConfig(level=logging.INFO) | |
| logger = logging.getLogger(__name__) | |
| logger.info("Hello from tus_storagehandler!") |
There was a problem hiding this comment.
I think this is a good comment, please add logging.
There was a problem hiding this comment.
Thank you for your feedback, @uniqueg. I'll go ahead and update the code to use the logging framework as suggested. This will provide better control over log levels and output destinations. I'll push the changes shortly.
There was a problem hiding this comment.
Yes agreed, but I think @psankhe28 should have selected "n" for add_script option while templating this proj, then this file wouldn't have been generated and nor will the console script would have been added, I don;t think we need a console script for this proj do we?
But if we do, then maybe do this in another PR(leave this to be templating only 🤷♂️ ), or I should add this in the cookiecutter itself.
PS: This is just an opinion tho :).
There was a problem hiding this comment.
Thank you for your input, @JaeAeich. You raise a valid point about the necessity of the console script for this project. If the console script is not required, it might indeed be more efficient to exclude it during the templating process with Cookiecutter.
However, since the file has already been generated and included in this PR, I suggest we proceed with the logging update for now to maintain consistency and ensure proper logging practices. We can then evaluate the need for the console script in a separate discussion or PR. If we decide it's unnecessary, we can remove it and update the Cookiecutter template accordingly.
Does that sound like a reasonable approach to everyone?
There was a problem hiding this comment.
@JaeAeich I had used the default value for add_script.
There was a problem hiding this comment.
Thank you for clarifying, @psankhe28. Given that the default value for add_script was used, it makes sense why this file was generated.
To move forward, I suggest we implement the logging update in this PR to ensure we follow best practices. After merging this PR, we can have a separate discussion to determine whether the console script is necessary for the project. If we conclude that it isn't, we can update the Cookiecutter template to exclude it and remove the file in a subsequent PR.
Does that sound like a good plan to everyone?
.cruft.json
Outdated
| "cookiecutter": { | ||
| "author_name": "Elixir Cloud AAI", | ||
| "author_email": "[email protected]", | ||
| "development_status": "Beta", |
There was a problem hiding this comment.
I think this should rather be "Alpha"
There was a problem hiding this comment.
@psankhe28 This has been changed to a list (sorry 😶🌫️ ), so this should now be something like
{
...
"commit": "43f9bd92eaf21557e87d3932524fe5ffb6fd7ef4",
"checkout": null,
"context": {
"cookiecutter": {
"author_name": "ELIXIR Cloud AAI",
"author_email": "[email protected]",
"development_status": "1 - Planning", <- or 3 - Alpha
"short_description": "A short description of the project.",
"project_name": "python-cookiecutter",
"project_slug": "python_cookiecutter",
....
}
There was a problem hiding this comment.
For the development_status, "development_status": "Alpha", is this right?
There was a problem hiding this comment.
No, I think you should use "1 - Planning", including the number. As you progress, you can change this to "2 - Pre-Alpha" and "3 - Alpha" as we see fit.
There was a problem hiding this comment.
Understood, thanks!
|
|
||
| def main(): | ||
| """Main entry point for tus_storagehandler.""" | ||
| print("Hello from tus_storagehandler!") |
There was a problem hiding this comment.
I think this is a good comment, please add logging.
|
Oh, and please make sure all of the tests pass 🙏 |
pyproject.toml
Outdated
| tus_storagehandler = "tus_storagehandler.main:main" | ||
|
|
||
| [tool.ruff] | ||
| indent-width = 2 |
There was a problem hiding this comment.
change it to 4 and run,
make v
source .venv/bin/activate
make fl
make pc
git add .
git commit -m "style: change indent"PS: The above command are meant to tell you what to do if they fail (like you use windows) then change the commads accordingly, you'll find the commands in Makefile.
There was a problem hiding this comment.
In readme, according to the make fl command, following changes should be done:
error: mke should be make
--> .\README.md:95:1
|
95 | mke = 'mke'
| ^^^
|
error: mke should be make
--> .\README.md:95:8
|
95 | mke = 'mke'
| ^^^
Should I do this?
There was a problem hiding this comment.
@uniqueg please review this workflow, before merge or either we should remove it from the cookiecutter too, this hasn't been tested.
There was a problem hiding this comment.
Looks good to me. Let's run it and see if it gives any errors. If so, we can update it until it works, then also change it in the cookiecutter.
|
@uniqueg test ci is failing because this repo doesn't have the token setup. |
Okay, I'll check that later. |
Signed-off-by: Prati28 <[email protected]>
Hey checkout test CI logs, it still shows that codecov token are missing. |
|
@uniqueg seems like I don't have permission to this repo, can you please add required permissions.
|
Done, @JaeAeich |
|
@JaeAeich: I think it makes much more sense if you review this PR as long as the Cookiecutter PR is still not merged, as it might require another round of back and forth with that one (which I'm already reviewing). Also, you are more familiar with the code and potential pitfalls. Is that okay? Let's ideally get this done this week (both the Cookiecutter PR and then also this one), so that @psankhe28 gets unblocked. |
|
Upon reviewing #1 and #9, I suppose they will also become redundant with the Cookiecutter template in place and with this PR updated, right, @JaeAeich and @psankhe28? That is, #9 becomes fully redundant (or alternatively this one in favor of #9) and from #1 only the non boilerplate code should be salvaged (but probably in a fresh PR that already has the boilerplate merged). It would be good to get to a point where we have only one of these PRs (appropriately named, perhaps |
|
Closed in favor of #10 |
#4
Summary by Sourcery
This pull request sets up the basic project structure using Cookiecutter, including Makefile, Sphinx documentation, GitHub Actions for CI/CD, a Code of Conduct, and initial test placeholders.