-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Migrate Find in files plugin to new API #12382
Conversation
7eb7c37
to
47d3a9f
Compare
b6dea2b
to
0f35664
Compare
Rebased and ready for review @ccordoba12 |
8aa2ec2
to
d21b943
Compare
Hi @dalthviz, this one is ready for QA testing when you have some time. Thanks! |
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.
After an initial test seems like all the actions work as expected. LGTM 👍
This one is ready for review @ccordoba12 |
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.
@goanpeca, some small comments for you and (unfortunately) tests are failing.
1215fef
to
696500a
Compare
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.
An error raises when running now (maybe a missing sig_
when connecting a signal from the Working Directory @goanpeca ?):
@dalthviz that has been fixed already https://github.com/spyder-ide/spyder/pull/12382/files#diff-449a26d3a4d2cdde344b13b43e1fb864R71, I think you needed to pull/refresh your local branch. |
Yep @goanpeca I was missing reseting my local with the latest changes 👍 Checking them I found this: |
Will look into it
Will fix!
May be related to the things reported on the Plots plugin? I fixed those issues on that PR. |
Maybe could be better to merge first the plots PR to get the fix implemented and then update this PR to check if the issue remains or is fixed? |
Ok, will look into that PR later today so we can merge that one first then this one. Thank you both for your help. |
This is ready for review @ccordoba12 |
Will fix! |
Fixed @dalthviz |
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.
@goanpeca, some small comments and then this should be ready.
Updated @ccordoba12. |
3d3e10d
to
1f0f58a
Compare
@goanpeca, this is ready. Please squash and rebase so I can merge it. |
The pip failures are unrelated. Merging. |
Description of Changes
Issue(s) Resolved
Fixes #12189
Affirmation
By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.
I certify the above statement is true and correct: @goanpeca