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

Fixed typos in docstring of subroutines.py #135

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Fil158
Copy link

@Fil158 Fil158 commented Nov 7, 2024

Replaces #126

No news (not required).

@sbillinge please review it.

Copy link

github-actions bot commented Nov 7, 2024

Warning! No news item is found for this PR. If this is a user-facing change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.

@bobleesj
Copy link
Contributor

bobleesj commented Nov 7, 2024

@Fil158

Thanks for using 'Closes'. However, we close an issue, not a PR. The one you've linked is a PR.

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.07%. Comparing base (3352045) to head (65a8618).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #135   +/-   ##
=======================================
  Coverage   71.07%   71.07%           
=======================================
  Files           9        9           
  Lines         242      242           
=======================================
  Hits          172      172           
  Misses         70       70           

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

@bobleesj
Copy link
Contributor

bobleesj commented Nov 7, 2024

@sbillinge ah, codecov upload is failing. It's being moody today.

@sbillinge
Copy link
Contributor

@Fil158

Thanks for using 'Closes'. However, we close an issue, not a PR. The one you've linked is a PR.

yes, this is correct. We do want to refernce this PR though. As an example, I might have written something like:

Closes #

> @Fil158
> 
> Thanks for using 'Closes'. However, we close an issue, not a PR. The one you've linked is a PR.

I fixed this.

@Tieqiong
Copy link

Tieqiong commented Nov 7, 2024

I think PR can closes PR. PR on Github are just special issues.

@sbillinge
Copy link
Contributor

I think PR can closes PR. PR on Github are just special issues.

this is true, but it makes it a bit harder to get the messaging correct. We normally close PRs manually when they are replaced by other PRs and say why, whilst linking them so we don't lose the conversation.

For issues we prefer automatic closing because the workflow is "I am going to open this PR to close this issue" and even if it takes 6 months, we are confident that when the PR is merged, the issue will close and we don't have to remember to go back and do it manually..... This is very different. We don't want multiple PRs open on the same thing, waiting for the second one to be merged, it only causes confusion (and the wrong one may get merged by mistake)

@bobleesj
Copy link
Contributor

bobleesj commented Nov 8, 2024

@sbillinge could you plz re-run the CI? codecov seems to be now stable again.

@sbillinge
Copy link
Contributor

@sbillinge could you plz re-run the CI? codecov seems to be now stable again.

I reran but it failed with a codecov issue. We may want to try pushing a new commit to clear out any cache?

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.

4 participants