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

Use declarative setup for SnapPy #101

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

magnusuMET
Copy link
Collaborator

No description provided.

@magnusuMET magnusuMET force-pushed the feature/setup.cfg branch 2 times, most recently from 4dcf294 to 61ab003 Compare November 2, 2022 09:12
Copy link
Member

@heikoklein heikoklein left a comment

Choose a reason for hiding this comment

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

Looks generally good.

I don't see any necessity to move all scripts from the SnapPy folder to the SnapPy/snapscripts folder. This change will destroy the environments which run scripts directly without complete installation, which is often the case.

Moving snapAddToa from the bsnap debian package to snap-py debian package destroys the distinction between those two packages (bsnap: command-line tools, snap-py: graphical user interface (heavy qt requirements)).

Too many heavy changes for a pure cleanup.

utils/SnapPy/snapscripts/snapEnsPlot.py Outdated Show resolved Hide resolved
@magnusuMET
Copy link
Collaborator Author

This does bring big changes, but helps reduce confusion in which package version one is running. It can be quite confusing when the import pulls in the installed package instead of the edited one. I find it more obvious to do this the pythonic way with pip install --editable. All places where scripts were being run directly has been changed to run from PATH afaik.

Is the distinction between bsnap and snap-py that obvious now? We have 8 programs under snap-py which does not depend on QT, in fact I have placed snapPy dependencies under extras. I think of bsnap as binary snap, a driver used by snapPy, and snapPy as all utilities supporting the model.

@heikoklein
Copy link
Member

It is true that the scripts under utils/SnapPy consists for two reasons:
Graphical usage
"snapPy",
"snapCombineInverse", (invoked best from graphical, never used it non-graphical, but might be possible, maybe should be moved to bsnap?)

Operational usage:
"snap4rimsterm",
"snapNc2grib.py",
"snapRunnerNpps",
"snapRunnerNpp",
"eemepModelRunner",
"snapRemoteRunner.py",

So the operational ones could be removed from the debian snap-py package.

But not enough reason to put them all into another directory.

@heikoklein heikoklein marked this pull request as draft May 19, 2023 10:35
@heikoklein
Copy link
Member

Rather than just moving to declarative setup.cfg, this PR requires many changes to the interna of the package. It hasn't been worked on in several month. And for a change to a declarative setup. PEP518/pyproject.toml (from 2016) should be used rather than setup.cfg. I suggest closing this PR and if still of interest, starting a new PR just replacing setup.py with pyproject.toml (for compatibility, please keep a boilerplate setup.py)

@magnusuMET
Copy link
Collaborator Author

The move to setup.cfg would greatly simplify conversion to pyproject.toml when python3.6 is phased out (pyproject.toml is not fully supported under 3.6)

Copy link
Member

@heikoklein heikoklein left a comment

Choose a reason for hiding this comment

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

This PR moves files, breaks in-place testing, changes filenames, breaks debian-packages and dependencies, without any benefit or reason except for transitioning to another build-system which we haven't even decided upon wanting to use. Please close silently.

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.

2 participants