Skip to content

skpkg: getting package up to scikit-package CI standards #340

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

Closed
wants to merge 9 commits into from

Conversation

cadenmyers13
Copy link
Contributor

Most files are default files modified by skpkg. no pre-commit edits were necessary.

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.

pls see comments

readme = "README.rst"
requires-python = ">=3.11, <3.14"
classifiers = [
'Development Status :: 5 - Production/Stable',
'Environment :: Console',
'Intended Audience :: Developers',
'Intended Audience :: Science/Research',
'License :: Free To Use But Restricted',
'License :: OSI Approved :: BSD License',
Copy link
Contributor

Choose a reason for hiding this comment

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

please check the license to make sure it is BSD before accepting this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbillinge confirmed BSD from skpkg

Copy link
Contributor

Choose a reason for hiding this comment

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

The relevant license is the diffpy.utils license not the skpkg license. What is the current diffpy.utils license.

@@ -16,14 +12,3 @@
# See LICENSE.rst for license information.
#
##############################################################################
"""diffpy - tools for structure analysis by diffraction.
Copy link
Contributor

Choose a reason for hiding this comment

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

its unlikely that we want to delete the contents of this file. But I am not sure. Is this a new development that we can handle the namespace strcture (diffpy.something) without this extend path? We may want to ask @bobleesj

Copy link
Contributor

Choose a reason for hiding this comment

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

An empty src/<namespacename>/__init__.py is still needed.

The namespace's``init.py, it's written in post_gen_hook.py` shown below:

def __gen_init__(module_name):
    """Generate __init__.py file for namespace module."""
    __init__ = f"""#!/usr/bin/env python
##############################################################################
#
# (c) {% now 'utc', '%Y' %} The Trustees of Columbia University in the City of New York.
# All rights reserved.
#
# File coded by: Billinge Group members and community contributors.
#
# See GitHub contributions for a more detailed list of contributors.
# https://github.com/{{ cookiecutter.github_username_or_orgname }}/{{ cookiecutter.github_repo_name }}/graphs/contributors
#
# See LICENSE.rst for license information.
#
##############################################################################

""" # noqa: E999
    return __init__

yield home_dir, cwd_dir


@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to delete all our tests?

@@ -1,6 +1,6 @@
"""Unit tests for __version__.py."""

import diffpy.utils
import diffpy.utils # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

It is probably ok to leave these # noqa's but they are not needed. I think we need them in the template because there is jinja code there that black can;t handle but after the template is built it would be better if they were deleted (but not hte end of theworld if they stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbillinge I'll investigate to see if there is a way to remove this. There might be a shell script we can run that removes this after the package is created. Something like grep -rn # noqa

Copy link
Contributor

Choose a reason for hiding this comment

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

no shell script, but maybe something in the post-gen-hook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bobleesj Do you have any insight on how this might be done?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add # noqa? The cookiecutter template doesn't require it:

"""Unit tests for __version__.py."""

import {{ cookiecutter.package_dir_name }}  # noqa


def test_package_version():
    """Ensure the package version is defined and not set to the initial
    placeholder."""
    assert hasattr({{ cookiecutter.package_dir_name }}, "__version__")
    assert {{ cookiecutter.package_dir_name }}.__version__ != "0.0.0"
``

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.

please see comment

@cadenmyers13 cadenmyers13 deleted the setup-CI branch June 10, 2025 21:17
Copy link
Contributor

@bobleesj bobleesj left a comment

Choose a reason for hiding this comment

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

@sbillinge @cadenmyers13 replied to your comments

@@ -16,14 +12,3 @@
# See LICENSE.rst for license information.
#
##############################################################################
"""diffpy - tools for structure analysis by diffraction.
Copy link
Contributor

Choose a reason for hiding this comment

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

An empty src/<namespacename>/__init__.py is still needed.

The namespace's``init.py, it's written in post_gen_hook.py` shown below:

def __gen_init__(module_name):
    """Generate __init__.py file for namespace module."""
    __init__ = f"""#!/usr/bin/env python
##############################################################################
#
# (c) {% now 'utc', '%Y' %} The Trustees of Columbia University in the City of New York.
# All rights reserved.
#
# File coded by: Billinge Group members and community contributors.
#
# See GitHub contributions for a more detailed list of contributors.
# https://github.com/{{ cookiecutter.github_username_or_orgname }}/{{ cookiecutter.github_repo_name }}/graphs/contributors
#
# See LICENSE.rst for license information.
#
##############################################################################

""" # noqa: E999
    return __init__

@@ -1,6 +1,6 @@
"""Unit tests for __version__.py."""

import diffpy.utils
import diffpy.utils # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add # noqa? The cookiecutter template doesn't require it:

"""Unit tests for __version__.py."""

import {{ cookiecutter.package_dir_name }}  # noqa


def test_package_version():
    """Ensure the package version is defined and not set to the initial
    placeholder."""
    assert hasattr({{ cookiecutter.package_dir_name }}, "__version__")
    assert {{ cookiecutter.package_dir_name }}.__version__ != "0.0.0"
``

@sbillinge
Copy link
Contributor

The # noqa is left over from hte template.(line 3)

@sbillinge
Copy link
Contributor

regarding the empty init, this was our question. Is just an empty init needed. Before in diffpy.utils we had some code about extending paths or something. I vaguely remember we managed to delete that and the namespace stuff all still worked, but I just wanted confirmation that my memory was correct, we don't need the delete "extend path" logic.

@bobleesj
Copy link
Contributor

The # noqa is left over from hte template.(line 3)

Ok. I see it.

Before in diffpy.utils we had some code about extending paths or something

Yes, we deleted it and it works without it - both via pytest after deployed to pypi/conda-forge.

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.

3 participants