Skip to content

Remove keyword duplication in named arguments - #39

Merged
abhro merged 1 commit into
mainfrom
terse-args
Jul 11, 2026
Merged

Remove keyword duplication in named arguments#39
abhro merged 1 commit into
mainfrom
terse-args

Conversation

@abhro

@abhro abhro commented Jul 11, 2026

Copy link
Copy Markdown
Member

No description provided.

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.15%. Comparing base (eeb599d) to head (4a5f65a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
- Coverage   46.17%   46.15%   -0.03%     
==========================================
  Files          17       17              
  Lines        2551     2550       -1     
==========================================
- Hits         1178     1177       -1     
  Misses       1373     1373              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@abhro
abhro merged commit bb40c9a into main Jul 11, 2026
6 checks passed
@abhro
abhro deleted the terse-args branch July 11, 2026 18:49
@barrettp

Copy link
Copy Markdown
Member

If this has been merged into main then it needs to be back out.

@abhro

abhro commented Jul 12, 2026

Copy link
Copy Markdown
Member Author

It passed the CI checks? I'm unsure what the issue is

@barrettp

Copy link
Copy Markdown
Member

Has anyone else reviewed it? I, in particular, would like to review changes to FITSFiles.jl, since I am the primary developer. I may have reasons for doing certain things that are not apparent to others. Not all PRs are appropriate.

@abhro

abhro commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

This PR and the PR removing trailing whitespace are... not appropriate? In general, I have been asking for reviews on PRs with substantial changes, but for such minor changes such as formatting and version updates for actions, I've been relying on the CI checks and using my judgment to determine if it should be safe to merge. At any rate, I see the CODEOWNERS file has been added, so this issue is now resolved, and similar situations like this will not be arising in the future in FITSFiles.jl. But I should state that this negates the idea of transferring a repository to JuliaAstro in the first place, if other members of the organization are not able to merge to main.

@barrettp

Copy link
Copy Markdown
Member

On the flip-side, if these PRs are not a big deal, then it should not be an issue to have someone else review and comment on them first, and then possibly do the merge. For example, the reason that I had not merged Jackson's PR was because I had not had the time to review it. Its functional interface almost certainly needs to be changed because it is not in-line with the compressed data API. Just because it can be merged doesn't mean that it should. Correct me if I am wrong, but it also appeared that the duplication in the named arguments removed the default values which should not have been done.

I think that the procedure for all JuliaAstro packages should be that all code PRs get a second review so that there is a consistent structure to the code, the exception being the primary developer. I just feel that it is common courtesy to do so. When I submit a PR to another project, I try to ensure that it is consistent with their programming style. The analogy that I give is: how would you like it if I walked into your house and started to rearrange the kitchen and the furniture? I doubt that you would be happy.

We should discuss this and similar issues at the next Steering Council meeting. Note that Astropy has leads for various projects whose job it is to review PRs for consistency and accuracy. We might want to consider this approach.

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