-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update _widget_reader.py #108
Conversation
Thanks, @TomasPetro ! I should have time to look this over later in the week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to figure out why the tests are stalling -- it's cause you added the QApplication instances. Napari handles all the main QT context, these need to be removed.
also, if you type |
remove Qapplucation Co-authored-by: Chris Havlin <[email protected]>
remove Qapplication Co-authored-by: Chris Havlin <[email protected]>
remove app.exec() Co-authored-by: Chris Havlin <[email protected]>
remove app.exec() Co-authored-by: Chris Havlin <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
==========================================
+ Coverage 99.42% 99.68% +0.25%
==========================================
Files 31 31
Lines 2431 2506 +75
==========================================
+ Hits 2417 2498 +81
+ Misses 14 8 -6 ☔ View full report in Codecov by Sentry. |
pre-commit.ci autofix
I looked at those comments and yes they were unnecessary features. I don't know why I put them there. I deleted them and tried them on my pc and everything went as before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the nice work, @TomasPetro !
I left some comments with some small changes to how you're setting the "$schema"
entry.
You'll also need to fix the failing style checks and add a new test.
style checks: you can read the full log here https://results.pre-commit.ci/run/github/448138442/1699393334.AOajljEzRPyJ5O8Rgft3mQ and manually fix the failures, or you can also type pre-commit.ci autofix
on its own in a comment and the bot will try to fix it for you (you'd then want to pull the changes locally).
new test: I did checkout your branch locally to make sure it works (and it does!) but when adding new features we also need to add to the automated test suite. So this PR will need a new test in src/yt_napari/_tests/test_widget_reader.py
. Adding tests for widget features can be a little tricky -- but you should be able to follow how the other tests are written to write a new one. In this case, I think the easiest route would be to monkeypatch the QFileDialog
exec_()
and selectedFiles()
functions (see this link for how to monkeypatch with pytest).
Updated schema
On my computer show 2 lines in github only 1
@chrishavlin |
Edit: Sorry, mixing up my repos. So yt-napari does still use tox. But I do still tend to just run Second Edit: There is a typo in the yt-napari docs. You start tox with just |
you'll need the |
I think the easiest approach will be to monkeypatch within pytest it so that you don't have to deal with the popup, see my comment up higher:
So you should be able to write a test that provides string inputs to provide a temporary filename and avoid the pop-up entirely. |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Hi, so I wrote the tests. Will you take a look at it ? Currently it doesn't pass only codecs/patches but I didn't do anything with those functions on those lines. |
Great! Thanks @TomasPetro ! I should be able to look later today or tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @TomasPetro ! I'll push up some changes to fix the coverage issues (as well as another couple small changes to the tests) and then this should be ready to merge.
ok, that's annoying... patch coverage is still failing. I'll push up one more change and just tell coverage to skip those lines (they're unrelated to this PR, not sure why they're getting picked up). |
Thanks for the work on this, @TomasPetro ! |
Nice :) |
I've edited it and it should work (I've tested saving the data and then loading it from File->OpenFile). I did this for YtReader and TimeSeriesReader. But the validate_data_model function does not return the model only the JSON. I don't know if it bothers or not but I had a problem converting the returned model to JSON. Besides, there is a hardcoded$schema$ because without it I couldn't load JSON files and I didn't know where I could get it into the function.