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

replace notify format usage with f-strings instead (#1409) (#1418) #1422

Merged
merged 2 commits into from
Mar 30, 2021
Merged

replace notify format usage with f-strings instead (#1409) (#1418) #1422

merged 2 commits into from
Mar 30, 2021

Conversation

itsabhianant
Copy link
Contributor

@itsabhianant itsabhianant commented Mar 29, 2021

Fixes #1418
Ref #1409

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #1422 (10e1d40) into latest (f4a6918) will increase coverage by 5.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest    #1422      +/-   ##
==========================================
+ Coverage   89.27%   94.49%   +5.21%     
==========================================
  Files         123       96      -27     
  Lines       18790    15176    -3614     
  Branches     1447     1447              
==========================================
- Hits        16775    14340    -2435     
+ Misses       1782      603    -1179     
  Partials      233      233              
Flag Coverage Δ
python 94.49% <100.00%> (ø)
rust ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/sourmash/search.py 91.22% <100.00%> (ø)
src/core/src/ffi/signature.rs
src/core/src/ffi/cmd/compute.rs
src/core/src/index/sbt/mhbt.rs
src/core/src/wasm.rs
src/core/src/from.rs
src/core/src/index/mod.rs
src/core/src/ffi/hyperloglog.rs
src/core/src/ffi/minhash.rs
src/core/src/sketch/nodegraph.rs
... and 18 more

Continue to review full report at Codecov.

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

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

hi @itsabhianant this looks good - should I review?

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

(you should "update branch", too, since we've merged something into latest since you started this)

@itsabhianant
Copy link
Contributor Author

Hi @ctb,

I have updated the branch as per your instructions.

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

ok! I'll wait for the tests to pass and then merge it.

request - could you edit the top comment to say something like,

Fixes https://github.com/dib-lab/sourmash/issues/1418
Ref #1409

so that the issues are linked? You did put the relevant issue numbers in the title of the pull request, but GitHub doesn't interlink the issues based on things in the title, it seems. Thanks!

@itsabhianant itsabhianant changed the title update call to notify in src/sourmash/search.py with f-strings (#1409) (#1418) Fixes https://github.com/dib-lab/sourmash/issues/1418 Ref #1409 Mar 30, 2021
@itsabhianant
Copy link
Contributor Author

itsabhianant commented Mar 30, 2021

Hi @ctb,

I don't know how to that as I am new to GitHub. I searched for it on the web but that didn't help. Could you please help me more briefly? Should we add links in the same way we do for README.md?

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

sure! I saw you changed the title, but I think the old title was mostly fine so you should change that back ;).

to edit the PR description, go to the '...' in the upper right of the first text box, just above "Checklist", and select "Edit".

@itsabhianant itsabhianant changed the title Fixes https://github.com/dib-lab/sourmash/issues/1418 Ref #1409 replace notify format usage with f-strings instead (#1409) (#1418) Mar 30, 2021
@itsabhianant
Copy link
Contributor Author

I updated the top comment as per your instructions @ctb . Is it fine or there are more changes to be made? Thanks for the help.

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

hi @itsabhianant, if you use the format here,

Fixes https://github.com/dib-lab/sourmash/issues/1418
Ref #1409

it will automatically close #1418 when this is merged, which is the preferred behavior. As it is, it properly references the two issues but won't close #1418.

(note you can use

Fixes #1418
Ref #1409 

too, github understands both the full link to the issue and just the issue number)

@itsabhianant
Copy link
Contributor Author

Hi @ctb,

I am getting an error while checking that displays:
There was a problem with Read the Docs while building your documentation.
I don't know what to do now...

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

no worries, that's not your fault! it's just a sporadic problem with RTD.

@itsabhianant
Copy link
Contributor Author

Oh, thanks...
I think the checks have completed if we are to ignore the docs issue.

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

yep - can you please update the comment at the top, tho, per #1422 (comment)? That way the relevant issue will be closed when I merge.

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

(you don't want the parantheses around the issue numbers. to see that you've got it right, look at the "linked issues" on the right where it says "Successfully merging this pull request may close these issues." Right now, it says "None yet." It should mention #1418)

@ctb ctb merged commit f3b0b8c into sourmash-bio:latest Mar 30, 2021
@itsabhianant
Copy link
Contributor Author

I have done that now it says:
This pull request closes issue #1418

Thanks for all the help sir. I learned a lot from you.

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

Thank you for your contribution! If you would like to be listed as a contributor in future publications, we will need your ORCID; you can get one here if you don't have one already.

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021

I have done that now it says:
This pull request closes issue #1418

Thanks for all the help sir. I learned a lot from you.

thank YOU!

if you are interested in trying out another issue, I'd suggest looking at one that involves editing/updating some test code. I can tag you in on two or three if you are interested. But no worries if not!

@itsabhianant
Copy link
Contributor Author

itsabhianant commented Mar 30, 2021 via email

@ctb
Copy link
Contributor

ctb commented Mar 30, 2021 via email

@itsabhianant
Copy link
Contributor Author

Hi @ctb,
This is my ORCID id:

ORCID

@itsabhianant
Copy link
Contributor Author

I have done that now it says:
This pull request closes issue #1418
Thanks for all the help sir. I learned a lot from you.

thank YOU!

if you are interested in trying out another issue, I'd suggest looking at one that involves editing/updating some test code. I can tag you in on two or three if you are interested. But no worries if not!

Yes, I would love to work on other issues. Please do tag me for editing/updating some test code.

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.

update call to notify in src/sourmash/search.py with f-strings
2 participants