-
Notifications
You must be signed in to change notification settings - Fork 18
feat: Add flags to replace wrong outputs and fill missing outputs #187
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
base: main
Are you sure you want to change the base?
Conversation
|
Note that this PR was produced using LLMs, with heavy editing after. |
|
Still todo:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #187 +/- ##
==========================================
+ Coverage 82.07% 82.57% +0.49%
==========================================
Files 28 29 +1
Lines 3549 3701 +152
Branches 736 775 +39
==========================================
+ Hits 2913 3056 +143
- Misses 504 507 +3
- Partials 132 138 +6 🚀 New features to boost your workflow:
|
|
@Erotemic Feel free to review even if it's still in draft state |
|
Overall it looks like the right place to add the new code. I've never been a huge fan of pytest fixtures, but with AI generated code I think they are fine. Some of the tests added look like they don't do anything: (e.g. We will definitely need tests for pathological cases. Ideally we should get the linter to pass, but I've not been great at maintaining type annotations, so if the mypy issue is truely out of scope we can ignore it. In a future PR I want to use AI to add proper inline type annotations, but based on my experience with other libraries, you have to go slow. It's not good enough to just fix an entire package at this point. I'm also not sure what the pypy error is. I'm testing CI here: #188 if it fails there too when we can conclude it's a different issue. In this PR you will also need to add a CHANGELOG entry. Add it to 1.3.1 for now. |
That's fair; in this case I feel like it could go either way; the code is clearer this way but it wouldn't be so hard to inline them. Let me know.
It turns out these were actual tests, in the form of doctests. I've moved them to their respective functions; sorry for the oversight.
I'd welcome any ideas you have for this; in the meantime I think I'll test the features manually and see if I like the outputs.
I'm actually not sure why the mypy check failed in my PR but not on main. I do in fact get the same errors on main, locally (ran with
The log feels quite far removed from what we're doing in this PR, so I'll keep it for later (if we deal with it at all).
You got it, thanks for the reminder. |
Closes #186