Skip to content

Conversation

@bobleesj
Copy link
Collaborator

@bobleesj bobleesj commented Apr 17, 2025

Level 4 - package create system

*The goal is to be able to work on a private GitHub repo with CI passing.
*User learns to use pyproject.toml
*User learns to use GitHub CI (no news check or codecov)

  • Pre-commit with flake8
  • Requirements divided into conda.txt, pip.txt, etc.
  • A simple README with installation instructions
  • No AUTHORS.rst, CODE_OF_CONDUCT.rst, MANIFEST.rst, LICENSE.rst, CHANGELOG.rst
  • No docs

Please see in-line comments.

Closes #3

Closes #6

"conda_pypi_package_dist_name": "{{ cookiecutter.project_name|replace(' ', '-')|lower }}",
"package_dir_name": "{{ cookiecutter.project_name|replace(' ', '_')|replace('-', '_')|lower }}",
"_copy_without_render": ["*.html", "*.js", ".github/*"]
}
Copy link
Collaborator Author

@bobleesj bobleesj Apr 17, 2025

Choose a reason for hiding this comment

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

No Python version specified - assume it's done via Python 3.13 or have the user create the conda environment based on the latest supported Python version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(We could ask them to check the latest Python version supported in the doc - which has been globally configured)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think assuming latest python is fine. Only rarely will this not be the best choice. When there are "special cases" we can alsways say that this case is not supported in level 4, so use level 5 if you need it, rather than doing all the work to have level 4 cover all the cases, if you see what I mean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done - noted in this repo's README.md

We assume that the latest Python version supported by skpkg is used in Level 4. If the user needs to specify the Python version, we encourage them to use Level 5.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just copied and pasted the file on handling namespace project structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Question: are we handling namespace structure at level 4? Strictly I would say that this is a complication that is not needed at level 4?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed, we don't need to support namespace in level-4. Also it's quite difficult to maintain this scary script as well in Level 5.

I removed the complicated logic in post_gen_project.py.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It appears that our test-on-pr currently requires CODECOV.. perhaps if they want to host this project on GItHub, maybe we can introduce them how to setup codecov and also pre-commit in Level 4 before Level 5.

Copy link
Contributor

Choose a reason for hiding this comment

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

mmm, not sure about that. Can we make a ligher-weight level 4 test on pr that doesn't require codecov. I want level 4 to be the gateway drug.....to be easy enough to set up that the people do it, then they get hooked on all the nice tools then they are prepared to put in the effort to set everything up at level 5. So anything that slows them down without adding real value at level 4 (such as codecov) is a downside I would say.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree pre-commit CI is very easy (just a few button clicks), while codecov CI can slow things down. Created an issue. #4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This readme could contain a quick guide on how to install the package locally. I will double-check this readme once I finish writing a tutorial for each Level of code sharing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of giving an empty file, I think having numpy in the text would be better. Also, numpy is used for the example code below.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we use np in example code, I think that is fine. In general we are then making things bloated, but this could be a good tradeoff as probably 90% of our target audience projects (scientists) will use numpy anyway. It is definitely better to have very clear examples.

tl;dr strong yes on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using the example code form Level 3 - I think having a concrete example below makes the starting process much eaiser like how journals provide latex templates. One of the students mentioned "where to start writing code.." scikit-package/scikit-package#282 - I think this example and the test code below indeed kinda of helps.

@bobleesj bobleesj marked this pull request as ready for review April 17, 2025 02:33
@bobleesj
Copy link
Collaborator Author

@sbillinge ready for review. CC' cadenmyers13

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

this looks good to me, modulo my inline comments. I think making reasonable decisions and merging something then testing it is the best. We can test it with some of the new students and see where they get stuck or we think things are too confusing.

I didn't mention inline, but I think I want a LICENSE.rst file even at this level. Even though this level is not really for sharing, I think getting in people's heads the importance of having a LICENSE file, and helping them by steering them towards bsd-3 clause, for example, is a good thing.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

This looks good The readme.txt that we wanted to delete seems still to be there and there are some missing CR's at the end of some files, but with those cleanings Let's merge and we can try it out, which is the best way to test it.

Copy link
Collaborator Author

@bobleesj bobleesj Apr 19, 2025

Choose a reason for hiding this comment

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

Remove namespace feature - very simple.

tests-on-pr:
uses: Billingegroup/release-scripts/.github/workflows/_tests-on-pr.yml@v0
secrets:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Leave CODECOV_TOKEN for now since it's a required parameter.. Created an issue #4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

License added

@bobleesj
Copy link
Collaborator Author

@sbillinge This is ready for review - since it's getting pretty big. We could merge this.

I am not so familiar with some missing CR's at the end of some files..

@sbillinge sbillinge merged commit e9861b8 into scikit-package:main Apr 19, 2025
@sbillinge
Copy link
Contributor

I merged per your request, but please could we make an issue to fix the tests. When they are fainalized on the other level we can copy-paste them over to here so they are the same.

@bobleesj
Copy link
Collaborator Author

I merged per your request, but please could we make an issue to fix the tests. When they are fainalized on the other level we can copy-paste them over to here so they are the same.

Issue created. #9

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.

docs: add LICENSE.rst file docs: move requirement's README.txt from cookiecutter package to docs?

2 participants