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.

Sign up for GitHub

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

Jump to bottom

DOC: f2py CLI documentation enhancements #25124

Merged
merged 6 commits into from
Nov 13, 2023
Merged

DOC: f2py CLI documentation enhancements #25124

merged 6 commits into from
Nov 13, 2023

Conversation

HaoZeke
Copy link
Member

@HaoZeke HaoZeke commented Nov 12, 2023 •
edited
Loading

f2py CLI documentation enhancements:

@HaoZeke HaoZeke requested a review from melissawm November 12, 2023 06:08
@HaoZeke HaoZeke changed the title MAINT: Kill 1.16 cliargs [f2py] [skip ci] DOC: f2py CLI documentation enhancements Nov 12, 2023
mattip
mattip reviewed Nov 13, 2023
View reviewed changes
numpy/f2py/auxfuncs.py Outdated
Comment on lines 6 to 7
Copyright 1999,2000 Pearu Peterson all rights reserved,
Pearu Peterson <pearu@ioc.ee>
Copyright 1999 - 2011 Pearu Peterson all rights reserved.
Copyright 2011 - 2023 NumPy Developers.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have precedent for adding another copyright line to modified files? I think this is covered by the overall project license, but if you feel strongly then perhaps we should simply remove the dates altogether, as suggested here to prevent additional yearly maintenance burden.

Copy link
Member Author

@HaoZeke HaoZeke Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we can / should remove Pearu's original copyright line, but at the same time I wanted to emphasize the current state of maintenance. Does 2012--present sound alright?

Edit: Note that the License.txt has both beginning (2005) and end (2023) dates.

mattip
mattip reviewed Nov 13, 2023
View reviewed changes
numpy/f2py/auxfuncs.py Outdated
Comment on lines 6 to 7
Copyright 1999 - 2011 Pearu Peterson all rights reserved.
Copyright 2011 - 2023 NumPy Developers.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove the years, they only add maintenance burden without really affecting the legal status of the statement (see the link above). So something like this, or even without the NumPy developers line (which is covered by the regular license).

Suggested change
Copyright 1999 - 2011 Pearu Peterson all rights reserved.
Copyright 2011 - 2023 NumPy Developers.
Copyright Pearu Peterson all rights reserved.
Copyright NumPy Developers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand the argument in the link, but here it would be nonsensical to have both all rights reserved under one name and also have the other. The years are important in the sense that it shows a transition in the copyright holders. So I would probably prefer 1999-2011 for Pearu and 2012 onwards or 2012--present for NumPy

Copy link
Member

Choose a reason for hiding this comment

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

IANAL. I think the whole change is not needed, but am open to accepting other views.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, it was a single commit I'm happy to revert, the rest of this really needs to be in ASAP, lots of confusion regarding the meson backend and f2py.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted for now, though I think its worth a discussion, opened #25137 for later.

@HaoZeke HaoZeke requested a review from mattip November 13, 2023 13:39
@HaoZeke HaoZeke mentioned this pull request Nov 13, 2023
Closed
@mattip
Copy link
Member

mattip commented Nov 13, 2023

Seems reasonable from what I can see. I am troubled by the lack of test coverage. Did you try this out both on distutils-based pre-3.12 and on meson?

@HaoZeke
Copy link
Member Author

HaoZeke commented Nov 13, 2023 •
edited
Loading

Seems reasonable from what I can see. I am troubled by the lack of test coverage. Did you try this out both on distutils-based pre-3.12 and on meson?

Yup, I tested (and passed CI with distutils) in the original implementation (#24532). Tests formeson required a rewrite of the testing interface, this was done in #25111 (just waiting on some of the other PRs). The switch from distutils to meson for tests makes sense in the larger context of meson being the build system for the CI (#23838) and is thus now always present. Plus we really want users to switch to meson ASAP.

The documented changes here are already in the code (in an ideal world they should have been part of the 1.26 release where the meson backend was introduced). We don't have very comprehensive tests for the CLI AFAIK, just for the functions themselves, which is why bugs like #25123 snuck in. CLI tests are a relatively recent feature, I started adding them (around 70% coverage) two years ago (#20668). The GSoC summer work included some front-end testing (#21835), but since the rewrite stalled (#21923) in favor of the backend work, tests were not covered. I think we ought to test more of the CLI. Should be a different issue though. Since documentation seems alright I guess it would be best to have it in, can be iterated on later.

@mattip mattip merged commit 5b1317c into numpy:main Nov 13, 2023
58 checks passed
@mattip
Copy link
Member

mattip commented Nov 13, 2023

Thanks @HaoZeke

@HaoZeke HaoZeke deleted the f2pyCLIcruft branch November 13, 2023 15:58
@rgommers rgommers added this to the 2.0.0 release milestone Nov 15, 2023
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Dec 24, 2023
@charris charris changed the title DOC: f2py CLI documentation enhancements DOC: f2py CLI documentation enhancements Dec 24, 2023
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Dec 24, 2023
@HaoZeke HaoZeke restored the f2pyCLIcruft branch June 15, 2024 23:40
@HaoZeke HaoZeke deleted the f2pyCLIcruft branch November 10, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melissawm melissawm Awaiting requested review from melissawm

@mattip mattip Awaiting requested review from mattip

Assignees
No one assigned
Labels
03 - Maintenance component: numpy.f2py
Projects
None yet
Milestone
2.0.0 release
Development

Successfully merging this pull request may close these issues.

BUG,DOC: F2PY CLI usage discrepancy
4 participants
@HaoZeke @mattip @charris @rgommers