Skip to content

docs: remove skpkg template files and mention of Brookhaven lab in index.rst #71

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

Merged
merged 3 commits into from
Aug 6, 2025

Conversation

zmx27
Copy link
Contributor

@zmx27 zmx27 commented Aug 5, 2025

What problem does this PR address?

Addresses the request in the following comment: #67 (comment)

What should the reviewer(s) do?

Please check my modifications.

pyproject.toml Outdated
@@ -58,7 +58,7 @@ exclude = [] # exclude packages matching these glob patterns (empty by default)
namespaces = false # to disable scanning PEP 420 namespaces (true by default)

[project.scripts]
pyobjcryst = "pyobjcryst.app:main"
pyobjcryst = "pyobjcryst.pyobjcryst_app:main"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pyobjcryst.app module does not exist. See the following screenshot:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

If no app exists should we simply remove this code-block ([project.scripts])?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that the pyobjcryst_app.py file is what the project.scripts is meant to call. It utilizes argparsers to parse CLI commands from the user and everything. Here are the contents of the file, so please let me know if you still want the [project.scripts] code block deleted!

import argparse

from pyobjcryst.version import __version__  # noqa


def main():
    parser = argparse.ArgumentParser(
        prog="pyobjcryst",
        description=(
            "Python bindings to the ObjCryst++ library.\n\n"
            "For more information, visit: "
            "https://github.com/diffpy/pyobjcryst/"
        ),
        formatter_class=argparse.RawDescriptionHelpFormatter,
    )

    parser.add_argument(
        "--version",
        action="store_true",
        help="Show the program's version number and exit",
    )

    args = parser.parse_args()

    if args.version:
        print(f"pyobjcryst {__version__}")
    else:
        # Default behavior when no arguments are given
        parser.print_help()


if __name__ == "__main__":
    main()

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just the standard one that is produced by scikit-package. I think we can delete this

@zmx27
Copy link
Contributor Author

zmx27 commented Aug 5, 2025

@sbillinge ready for review

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 my inline comments. Also, please could you go through the readme and make sure everything there is correct?

pyproject.toml Outdated
@@ -58,7 +58,7 @@ exclude = [] # exclude packages matching these glob patterns (empty by default)
namespaces = false # to disable scanning PEP 420 namespaces (true by default)

[project.scripts]
pyobjcryst = "pyobjcryst.app:main"
pyobjcryst = "pyobjcryst.pyobjcryst_app:main"
Copy link
Contributor

Choose a reason for hiding this comment

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

If no app exists should we simply remove this code-block ([project.scripts])?

@zmx27
Copy link
Contributor Author

zmx27 commented Aug 6, 2025

@sbillinge ready for review

@sbillinge sbillinge merged commit 2e22303 into diffpy:migration Aug 6, 2025
2 checks passed
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