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

Require passing parameters by name not position in many methods and functions #1333

Merged
merged 12 commits into from
Oct 16, 2023

Conversation

bhazelton
Copy link
Member

@bhazelton bhazelton commented Aug 28, 2023

Description

Require keyword arguments rather than allowing for passing arguments by position for functions & methods with many parameters. This should help users as well as ensuring that the developers can re-order input parameters or add new ones in any position without making a breaking change.

Deprecation is not really possible for this, so it's going into version 3.0.

Motivation and Context

closes #1223

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change (documentation changes only)
  • Version change
  • Build or continuous integration change

Checklist:

Breaking change checklist:

  • I have updated the docstrings associated with my change using the numpy docstring format.
  • I have updated the tutorial to reflect my changes (if appropriate).
  • My change includes backwards compatibility and deprecation warnings (if possible).
  • I have added tests to cover my changes.
  • All new and existing tests pass.
  • I have updated the CHANGELOG.

@bhazelton bhazelton changed the title Params by name Require passing parameters by name not position in many methods and functions Aug 28, 2023
Copy link
Member

@mkolopanis mkolopanis left a comment

Choose a reason for hiding this comment

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

I think we're off to a good start here. There are some inconsistencies I've pointed out and one spot I personally feel like we're being a little over zealous with requiring everything to be a keyword argument.

also a reminder that test_no_moon is still failing on most wheel builds for pypi.

pyuvdata/uvdata/initializers.py Outdated Show resolved Hide resolved
pyuvdata/uvdata/uvdata.py Show resolved Hide resolved
pyuvdata/uvdata/uvdata.py Outdated Show resolved Hide resolved
pyuvdata/uvdata/uvfits.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #1333 (f7f3ada) into prep_v3.0 (179e7ee) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           prep_v3.0    #1333   +/-   ##
==========================================
  Coverage      99.92%   99.92%           
==========================================
  Files             36       36           
  Lines          20334    20337    +3     
==========================================
+ Hits           20318    20321    +3     
  Misses            16       16           
Files Coverage Δ
pyuvdata/parameter.py 100.00% <100.00%> (ø)
pyuvdata/utils.py 100.00% <100.00%> (ø)
pyuvdata/uvbase.py 100.00% <ø> (ø)
pyuvdata/uvbeam/beamfits.py 100.00% <100.00%> (ø)
pyuvdata/uvbeam/cst_beam.py 100.00% <100.00%> (ø)
pyuvdata/uvbeam/mwa_beam.py 100.00% <100.00%> (ø)
pyuvdata/uvbeam/uvbeam.py 100.00% <100.00%> (ø)
pyuvdata/uvcal/calfits.py 100.00% <ø> (ø)
pyuvdata/uvcal/fhd_cal.py 100.00% <ø> (ø)
pyuvdata/uvcal/initializers.py 100.00% <100.00%> (ø)
... and 11 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 179e7ee...f7f3ada. Read the comment docs.

Copy link
Contributor

@steven-murray steven-murray left a comment

Choose a reason for hiding this comment

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

Thanks @bhazelton this is looking good. Just a couple of preferences, but nothing major.

docs/uvcal_tutorial.rst Outdated Show resolved Hide resolved
pyuvdata/utils.py Outdated Show resolved Hide resolved
pyuvdata/utils.py Outdated Show resolved Hide resolved
pyuvdata/utils.py Outdated Show resolved Hide resolved
@bhazelton bhazelton linked an issue Sep 12, 2023 that may be closed by this pull request
mkolopanis
mkolopanis previously approved these changes Sep 28, 2023
Copy link
Member

@mkolopanis mkolopanis left a comment

Choose a reason for hiding this comment

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

Finished another look. I don't think I have any more comments but since this is so big I'm not sure I want to hit the button too 😆 I'll approve of what we have but if others have comments that's good too.

@bhazelton bhazelton dismissed mkolopanis’s stale review October 15, 2023 22:45

The merge-base changed after approval.

@bhazelton bhazelton force-pushed the params_by_name branch 2 times, most recently from 044ee32 to 05fcba8 Compare October 16, 2023 00:05
Copy link
Member

@mkolopanis mkolopanis left a comment

Choose a reason for hiding this comment

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

I'll re-approve after the update. Doesn't look like much has changed on this front since my last view. Going to move forward with this PR.

@mkolopanis mkolopanis merged commit 4ce7a48 into prep_v3.0 Oct 16, 2023
41 of 44 checks passed
@mkolopanis mkolopanis deleted the params_by_name branch October 16, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change some long method signatures to require keyword arguments?
3 participants