-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Annotations extras #13228
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
Annotations extras #13228
Conversation
for more information, see https://pre-commit.ci
…ne-python into annotations-details
@drammock I implemented here the recommendations in your #13213 (comment) |
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.
just a quick first pass with some high-level stuff, LMK when I should take another look @PierreGtch
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 am onboard with the change although I am not convinced about the word "details"
Co-authored-by: Alexandre Gramfort <[email protected]> Co-authored-by: Daniel McCloy <[email protected]>
The failed tests seem unrelated to the PR (Read timed out) |
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 sticking with this @PierreGtch. I'm pretty happy with where this ended up. Approving (modulo one docstring typo) but would like to wait on merging until at least one other maintainer has had a look (mostly because I've only read the code, haven't actually had time to play around with the feature interactively, and tomorrow I'm off to a conference for a week so I can't do it soon)
Co-authored-by: Daniel McCloy <[email protected]>
thanks @drammock for your time on this PR! |
@sappelhoff @larsoner would you have time to also take a look? (the failed test seems unrelated to the PR) |
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 good! Just minor stuff from me really. Can you push the next commit with [circle full]
in the commit message? That will make CircleCI build all examples, which I think would be a nice sanity check (and save us some possible post-merge pain in case we missed something)
Co-authored-by: Eric Larson <[email protected]>
for more information, see https://pre-commit.ci
105 CircleCI failures (!). Luckily they're all of 2 types:
|
Thanks for the quick fix for the bug! For TDD though it's better if we had a test in |
for more information, see https://pre-commit.ci
@larsoner thanks for the review! |
The failed test seems unrelated |
Looking at what you had to change in 7406948, I think tests that called |
@drammock @larsoner The issue was introduced in the last-minute suggestion from code review: 9e67e3b |
haha, you're right. I was so focused on the CircleCI results that I didn't notice the copious failures in the pytest suite. I'll revert the new test then (since we already had the needed coverage) and mark for merge-when-green. Thanks @PierreGtch ! |
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
Reference issue (if any)
This PR replaces #13213
What does this implement/fix?
The difference with #13213 is that the additional information is a
list[dict[str, str | int | float | None]]
instead of a dataframe