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 History module #4593

Merged
merged 9 commits into from
Jun 16, 2017
Merged

Conversation

rlaverde
Copy link
Member

  • Move history to a module
  • Also decouple the plugin UI in a widget to make it reusable and easy testable.
history/
├── confpage.py
├── __init__.py
├── plugin.py
├── tests
│   └── test_history.py
└── widgets.py

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

goanpeca commented Jun 12, 2017

@rlaverde please fix docstrings. They should end in a period and use a single line if they fit. Always triple quotes

"""Something something something."""

or

"""
Something something something.

Extra something.
"""

@ccordoba12
Copy link
Member

@rlaverde this PR doesn't contain any renames, so git won't be able to detect the right file when we want to merge changes from 3.x.

So I think you need to redo this PR and use renames instead.

@ccordoba12
Copy link
Member

By renames, I mean git mv

@ccordoba12 ccordoba12 changed the title PR: History module PR: Split History module Jun 13, 2017
@rlaverde
Copy link
Member Author

rlaverde commented Jun 13, 2017

There was only one file, I move it in 23a0615

but after that I move some parts of it:
to confpage: b904374 (as in other plugins modules)
to widgets: 59b3d19

because of that github doesn't detect the rename, and changes in the code moved to other files will have to be merged manually

@ccordoba12
Copy link
Member

Ok, we need to be careful with changes to this plugin then.

from spyder.widgets.sourcecode import codeeditor
from spyder.config.base import _
from spyder.utils import icon_manager as ima
from spyder.utils.qthelpers import (add_actions, create_toolbutton)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parenthesis are not needed here.

@ccordoba12
Copy link
Member

I left a minor comment, then I'll merge.

@ccordoba12 ccordoba12 merged commit fdb148c into spyder-ide:split-plugins Jun 16, 2017
@rlaverde rlaverde deleted the history-module branch June 16, 2017 14:06
@ccordoba12 ccordoba12 modified the milestones: v4.0beta2, v4.0beta1 Jul 26, 2017
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