Skip to content

Conversation

@DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Jan 16, 2026

Summary by CodeRabbit

  • Chores
    • Added explicit Python support >=3.10, <3.13 (includes 3.12).
    • Upgraded packaging/build requirement to flit_core >=3.12.
    • Clarified project metadata and consolidated authorship contact.
    • Added BSD-3-Clause license declaration and included license file.
    • Added official project homepage URL.
    • Declared optional dependencies for visualization (Bokeh, Matplotlib, Plotly) and documentation tooling; updated the combined "all" group.

✏️ Tip: You can customize this high-level summary in your review settings.

Copilot AI review requested due to automatic review settings January 16, 2026 13:06
@coderabbitai
Copy link

coderabbitai bot commented Jan 16, 2026

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Updated pyproject.toml: bumped build-system flit_core to >=3.12,<4; replaced single author fields with an authors list and added project.urls; removed module, dist-name, and readme-content-type; added license metadata; tightened requires-python; expanded optional dependency groups (bokeh, matplotlib, plotly, docs).

Changes

Cohort / File(s) Summary
Project metadata & build system
pyproject.toml
Updated [build-system].requires to flit_core >=3.12,<4; replaced author/author-email with authors list; removed module, dist-name, and readme-content-type; added license = "BSD-3-Clause" and license-files = ["LICENSE"]; added [project.urls] Homepage; updated requires-python to >=3.10, <3.13; adjusted classifiers (added Python 3.12; removed BSD license classifier).
Optional dependencies
pyproject.toml
Added per-feature optional dependency groups: bokeh = ["bokeh>=3.4.1"], matplotlib = ["matplotlib"], plotly = ["plotly"], docs = [...]; updated all group to include bokeh, plotly, and matplotlib.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I nibbled TOML fields in evening's glow,
Authors rearranged in tidy row,
License tucked and python bounds refined,
Extras added so plots can be combined,
A tiny hop — configuration aligned.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Update packaging' is vague and overly generic. While it relates to the changeset (modifications to pyproject.toml), it does not convey what specific packaging aspect was updated or why. Consider a more specific title such as 'Update pyproject.toml metadata and dependencies' or 'Update build system and project metadata configuration' to better describe the actual changes.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR modernizes the pyproject.toml configuration to align with current PEP 621 standards. The changes migrate deprecated metadata fields to their modern equivalents and add support for newer Python versions.

Changes:

  • Updated flit_core minimum version from 3.2 to 3.12.0
  • Migrated deprecated author fields to modern authors array format
  • Moved homepage URL to new [project.urls] section
  • Added Python 3.12 and 3.13 classifiers and updated license specification

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"Topic :: Scientific/Engineering :: Chemistry",
]
license = "BSD-3-Clause"
license-files = ["LICENSE"]
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The license-files field is redundant when using the SPDX license identifier format. When license is specified as a string (SPDX identifier), the license-files field is not needed according to PEP 621. Either use license = {text = \"BSD-3-Clause\"} with license-files, or use just license = \"BSD-3-Clause\" without license-files.

Suggested change
license-files = ["LICENSE"]

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@DimitriPapadopoulos DimitriPapadopoulos Jan 16, 2026

Choose a reason for hiding this comment

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

Where does this come from? It's a bad idea and it's not compliant with PEP 639.

See "Use a default value for license-files if not specified" in Appendix: Rejected Ideas of PEP 639.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Around line 7-11: The authors array currently contains two separate entries
(one with {name} and one with {email}) which yields separate authors; update the
authors array so the contact is a single table entry containing both keys (e.g.,
combine into one {name = "...", email = "..."} object) so the metadata has one
author with a contact email; edit the authors block in pyproject.toml to replace
the two entries with a single combined table.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 29e0e80 and 8fa9c37.

📒 Files selected for processing (1)
  • pyproject.toml
🔇 Additional comments (1)
pyproject.toml (1)

2-2: No action needed. The flit_core>=3.12.0 version requirement is correct—flit_core 3.12.0 explicitly added support for PEP 639 SPDX license expressions and the license-files field used in this pyproject.toml (lines 25–26).

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Copilot AI review requested due to automatic review settings January 16, 2026 14:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

readme = "README.md"
readme-content-type = "text/markdown"
classifiers = [
"Intended Audience :: Science/Research",
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The 'License :: OSI Approved :: BSD License' classifier was removed but not replaced with a more specific classifier. Consider adding 'License :: OSI Approved :: BSD License' back to the classifiers list to maintain discoverability on PyPI, as classifiers serve a different purpose than the license field.

Suggested change
"Intended Audience :: Science/Research",
"Intended Audience :: Science/Research",
"License :: OSI Approved :: BSD License",

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@DimitriPapadopoulos DimitriPapadopoulos Jan 16, 2026

Choose a reason for hiding this comment

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

Your suggestion is not compliant with current Python packaging standards and PyPi is perfectly aware of the license field.

And where does "classifiers serve a different purpose than the license field" come from? Please provide links to authoritative documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PyPI moved to license in 2024:
pypi/warehouse#16620 (comment)

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 2: Update the pyproject.toml requires entry to reflect the true minimum
for PEP 639 support or document the reason for the higher pin: either change the
requires line from "flit_core >=3.12.0,<4" to "flit_core >=3.11.0,<4" if you
want the strict minimum that added PEP 639 support, or keep ">=3.12.0,<4" but
add a brief comment near the requires declaration explaining that 3.12.0 was
chosen for additional SPDX AND/OR and license-file handling improvements rather
than being the minimal PEP 639 requirement.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@jcharkow jcharkow left a comment

Choose a reason for hiding this comment

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

Looks good on my end.

pyproject.toml Outdated
author = "OpenMS Team"
authors = [
{name = "OpenMS Team", email = "[email protected]"},
{name = "Joshua Charkow", email = "[email protected]"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can likely remove my name/email and just keep the OpenMS team in this new format

@jcharkow
Copy link
Collaborator

@singjc do you know why the deploy documentation is failing? It seems to be related to your recent PR changing to personal access token.

Copilot AI review requested due to automatic review settings January 19, 2026 18:04
@singjc
Copy link
Collaborator

singjc commented Jan 19, 2026

@singjc do you know why the deploy documentation is failing? It seems to be related to your recent PR changing to personal access token.

I was testing using a different secuirty token, because I was running into permission issues with the deploy documentation action, for PRs made from my fork. Which I didn't get from PRs made from the upstream repo. But it doesn't seem to help, or the token doesn't have the right permissions..

I changed it back to the original GITHUB_TOKEN token

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@singjc
Copy link
Collaborator

singjc commented Jan 19, 2026

Yeah, it still seems to fail, I was getting this error too

 remote: Permission to OpenMS/pyopenms_viz.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/OpenMS/pyopenms_viz/': The requested URL returned error: 403
Error: Process completed with exit code 128.

The error doesn't occur if you're making a PR from the main repo, only from forks it seems

@jcharkow
Copy link
Collaborator

Yeah, it still seems to fail, I was getting this error too

 remote: Permission to OpenMS/pyopenms_viz.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/OpenMS/pyopenms_viz/': The requested URL returned error: 403
Error: Process completed with exit code 128.

The error doesn't occur if you're making a PR from the main repo, only from forks it seems

I could not get that far even, I run into an error with personal access tokens.

It seems that it is a permissions thing which is causing it to fail then.

It seems that this PR is good for the most part though so should be good to merge?

1 similar comment
@jcharkow
Copy link
Collaborator

Yeah, it still seems to fail, I was getting this error too

 remote: Permission to OpenMS/pyopenms_viz.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/OpenMS/pyopenms_viz/': The requested URL returned error: 403
Error: Process completed with exit code 128.

The error doesn't occur if you're making a PR from the main repo, only from forks it seems

I could not get that far even, I run into an error with personal access tokens.

It seems that it is a permissions thing which is causing it to fail then.

It seems that this PR is good for the most part though so should be good to merge?

@singjc
Copy link
Collaborator

singjc commented Jan 19, 2026

Yeah looks good, will merge. Will have to look into the issue with Deploy Documentation in another PR.

@singjc singjc merged commit 8e6c9a6 into OpenMS:main Jan 19, 2026
8 of 10 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.

3 participants