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

Refactor for src style package #79

Closed
Mark2000 opened this issue Oct 31, 2023 · 2 comments · Fixed by #88
Closed

Refactor for src style package #79

Mark2000 opened this issue Oct 31, 2023 · 2 comments · Fixed by #88
Labels
infrastructure Testing, documentation, and CI system refactor Refactor only. No new features.

Comments

@Mark2000
Copy link
Contributor

Mark2000 commented Oct 31, 2023

src directory style Python packages are preferred these days, so we should be using that. What are the specific benefits?

Considerations

  • Make a clean break - sooner is better than later, but hopefully doesn't mess up branches in development (does handle this sort of refactor well). If waiting for the first release (alpha->beta) is better for this transition, lets do it then.
  • Can we use a pyproj.toml like the cool kids do these days? Ensure it doesn’t break cluster install or how chebpy is installed.
@Mark2000 Mark2000 added enhancement New feature or request infrastructure Testing, documentation, and CI system refactor Refactor only. No new features. and removed enhancement New feature or request labels Oct 31, 2023
@Mark2000 Mark2000 mentioned this issue Nov 1, 2023
7 tasks
@johnowagon
Copy link
Contributor

johnowagon commented Nov 9, 2023

Some benefits to this structure:

  1. The main benefit is that src style is explicit, theres no ambiguity on what is available to import. Plus I think its cleaner.
  2. As of PEP 518, using pyproject.toml is recommended anyways.
  3. Not to mention configuring Sphinx will be much easier if this is done prior to setup.

From some initial research I don't think refactoring the structure is going to cause issues during rebases, however I haven't tested this yet, I'll jump on it tomorrow.

Side note, because the setup.py script ran some code along with installation, I had to specify an entrypoint in the pyproject.toml to achieve the same functionality. This changes the install command from pip install -e . to pip install -e . && finish-install.

@Mark2000
Copy link
Contributor Author

Mark2000 commented Nov 9, 2023

Even though the install command would be longer/less simple, it'd probably better. I could imaging someone not liking that setup.py has calls to sys and downloads files from the internet with no warning; finish-install makes that more explicit.

We will want to test that pyproject.toml-based install works on the on the CURC cluster. I tried that with another small package I made for a class and couldn't get it to work due to the build-system specification, but didn't try very hard to find a fix to the issue. @johnowagon, @LorenzzoQM should be able to help you get an account on the cluster and get Basilisk installed there, he did it fairly recently and made some fixes to my broken cluster install script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Testing, documentation, and CI system refactor Refactor only. No new features.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants