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

PR: Split Find in Files module #4569

Merged
merged 7 commits into from
Jul 26, 2017

Conversation

rlaverde
Copy link
Member

@rlaverde rlaverde commented Jun 6, 2017

findinfiles/
├── __init__.py
├── plugin.py
├── tests
│   └── test_findinfiles.py
└── widgets.py

@rlaverde rlaverde added this to the v4.0beta1 milestone Jun 6, 2017
@rlaverde rlaverde self-assigned this Jun 6, 2017
@rlaverde rlaverde requested a review from ccordoba12 June 6, 2017 22:08
@goanpeca
Copy link
Member

goanpeca commented Jun 7, 2017

@rlaverde what is missing here to get tests to pass?

@rlaverde
Copy link
Member Author

rlaverde commented Jun 7, 2017

what is missing here to get tests to pass?

I forget to move the data used in the tests 🙈

@ccordoba12
Copy link
Member

I'd like to make some more improvements to Find in Files before 3.2 is released, so I think we should hold this one until we release 3.2.

@rlaverde
Copy link
Member Author

I'd like to make some more improvements to Find in Files before 3.2 is released, so I think we should hold this one until we release 3.2.

Should I merge master into split-pĺugins, and after that rebase this PR? or how should I bring that changes?

@ccordoba12
Copy link
Member

Should I merge master into split-pĺugins, and after that rebase this PR?

Yep, I think that's what you need to do.

@rlaverde rlaverde force-pushed the find-in-files-module branch from c6368d0 to 3983512 Compare July 24, 2017 19:11
@rlaverde
Copy link
Member Author

rlaverde commented Jul 25, 2017

@ccordoba12 This is ready, I rebased it after the merging of split-plugins with master

@ccordoba12 ccordoba12 changed the title PR: Find in files module PR: Split Find in files module Jul 25, 2017
@ccordoba12
Copy link
Member

@rlaverde, let's take the opportunity to move the classes of widgets.py to different files, like this:

  • SearchThread -> utils/search.py
  • FindOptions -> widgets/options.py
  • LineMatchItem, FileMatchItem, ItemDelegate, ResultsBrowser -> widgets/results.py
  • FileProgressBar, FindInFilesWidget -> widgets/main.py

I know this will make merging PRs from 3.x to master much harder, but it's better to do things right in 4.0 to avoid this kind of refactoring in the future.

@ccordoba12
Copy link
Member

ccordoba12 commented Jul 26, 2017

I've been thinking on my last suggestion. So, the problem with splitting things further and still maintaining the 3.x branch is that it would make merging from 3.x to master or from master to split-plugins a real nightmare!

So a further splitting should be done only after we don't plan to do more 3.2.x releases (probably in six months or so).

@rlaverde, for now, please continue to do things as you've been doing them ;-)

@ccordoba12 ccordoba12 modified the milestones: v4.0beta2, v4.0beta1 Jul 26, 2017
@ccordoba12 ccordoba12 changed the title PR: Split Find in files module PR: Split Find in Files module Jul 26, 2017
@ccordoba12 ccordoba12 merged commit 0e1a701 into spyder-ide:split-plugins Jul 26, 2017
@rlaverde
Copy link
Member Author

So a further splitting should be done only after we don't plan to do more 3.2.x releases (probably in six months or so).

Well, I will open another PR with the split, when this happen, although I think the "double copied merging" trick will work for merging changes (I did that in #4812)

@rlaverde rlaverde deleted the find-in-files-module branch July 26, 2017 17:11
@ccordoba12
Copy link
Member

I think the "double copied merging" trick will work for merging changes

Great! But just in case, let's wait until January or so to make further splittings. I think that's not that bad because it's simply more refactoring ;-)

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.

3 participants