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

[MRG] add drive warnings #516

Merged
merged 7 commits into from Oct 1, 2022
Merged

Conversation

orbekolo
Copy link
Contributor

Closes #269

@ntolley
Copy link
Contributor

ntolley commented Jul 20, 2022

@orbekolo this would be a good place to add a test for checking if the warning message is raised:

with pytest.warns(UserWarning, match='No connections'):

This function checks if a warning is raised when there are no connections in the local network. You can modify the same code to check if your warning for the drives is raised. match is just a criteria for the test to pass, as in some part of the warning message must contain that string.

After that function indicated, you can use net.add_connection() to add any connection (and avoid the no connections warning). On the next line you can add the with pytest.warns..., and inside the test you can run simulate_dipole(). The drives warning should be raised since net.clear_drives() is called just a few lines up.

@jasmainak
Copy link
Collaborator

have you read the contributing guide: https://github.com/jonescompneurolab/hnn-core/blob/master/doc/contributing.rst

As you are going through the process, it would be great to have your feedback on this and potentially improve upon it

@jasmainak
Copy link
Collaborator

@orbekolo just checking in here. Do you need help? We're going to revamp the contributing guide ... any feedback would be most welcome!

@orbekolo orbekolo changed the title WIP: add drive warnings [MRG] add drive warnings Sep 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2022

Codecov Report

Merging #516 (0ca7aba) into master (908c818) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 0ca7aba differs from pull request most recent head 150f21e. Consider uploading reports for the commit 150f21e to get more accurate results

@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
- Coverage   91.43%   91.41%   -0.02%     
==========================================
  Files          22       21       -1     
  Lines        4157     4149       -8     
==========================================
- Hits         3801     3793       -8     
  Misses        356      356              
Impacted Files Coverage Δ
hnn_core/dipole.py 92.50% <100.00%> (+0.09%) ⬆️
hnn_core/__init__.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ntolley
Copy link
Contributor

ntolley commented Sep 29, 2022

@orbekolo for some reason the changes to whats_new.rst didn't get updated here. You may need to try git commit-ing the file again and push again.

doc/whats_new.rst Outdated Show resolved Hide resolved
Co-authored-by: Nicholas Tolley <55253912+ntolley@users.noreply.github.com>
@jasmainak
Copy link
Collaborator

@chenghuzi I let you merge if you're happy!

@chenghuzi
Copy link
Collaborator

@chenghuzi I let you merge if you're happy!

LGTM!

@chenghuzi chenghuzi merged commit dcafdbb into jonescompneurolab:master Oct 1, 2022
@ntolley
Copy link
Contributor

ntolley commented Oct 1, 2022

@orbekolo congrats on the first PR merged! 🎉 🎉

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.

raise warning when no external drives added
5 participants