-
-
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
WIP: Add extended VCS support with Git implementation #13562
Conversation
Hello @trollodel! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-01-01 13:17:02 UTC |
Hey @trollodel, thanks a lot for your contribution! It looks really great! One small comment before you proceed further: you changed the line endings of And thanks again for your work on this. It's going to be a great addition to Spyder 5! |
6da1f9d
to
167af25
Compare
d82bdf6
to
e9fe1c1
Compare
Update 2Added commit history with a button (if supported) for undo the commit. You can also create a new branch by typing its name into the branches combobox, but it has some bugs that make the creation always fail. Dev changesI complete the API specification (the class VCSBackendBase in vcs/utils/api.py) that I want to include in this PR, except for error handling that requires dedicated work (located in vcs/utils/errors.py). |
dac45e3
to
de94302
Compare
Update 3 (backend features freeze)After this update, I stop developing new features on VCSBackendBase. Changes context menuWith a refractor of changes widgets, I introduce a context menu for changes items. When the change is unstaged (as in the screenshot) you can also discard the change (with no possibilities of recovering it). Create dialogNow, when the project has no repository in it, the following screen shows in the VCS pane: If you press the button, the following dialog shows: Here you can choose the VCS type and an optional URL for a remote repository (like git clone). |
Thanks for your hard work on this new plugin @trollodel! It's going to be a fantastic addition to Spyder 5! Would you like to see your work added to our source code? Or do you prefer to maintain it as a separate plugin? |
I surely want to see this PR merged. But I will continue to develop it in my repository until this PR is close to be merged. |
Ok, that's what I thought but I wanted to confirm with you.
Great! When you reach that point, I propose to break your work at least in two parts (I don't know if more are feasible) to make the review process easier:
What do you think? |
It's pretty easy to do. But,
|
Good thinking, that's a good approach.
I agree with that too and also with having three PRs. |
Another option (that just occurred to me) is to move the repository you have on Gitlab to our organization here, so you can keep developing independently of our main repo for some time. And when it's stable enough, we could move it to the core of Spyder. That's the approach taken by the JupyterLab guys and it seems to work well for them. |
For me, It can be done (If I can have write access to it), but I don't understand why. |
Hi @trollodel thanks for the work on this! While checking I detected a couple of issues/elements that maybe could be improved (testing on Windows):
|
Also, the idea here is for this to be a third-party plugin (so it will live in a different repo for example: https://gitlab.com/trollodel/spyder-vcs/-/tree/master/ and be an installable PyPI package), or add it as a base plugin of Spyder? What do you guys think @spyder-ide/core-developers ? |
Thinking for a bit, a couple of options (maybe someone else has more ideas/options):
|
|
This is not an exception, but a debug
Strange, I just use an UrlCombobox.
I already know that it will cause problems. Now it is in my TODO list.
Will fix.
It is expected because it has no auto update.
Will fix.
|
Ok 👍 although IMO maybe could be worthy to check the external plugin option but lets wait if someone else has any opinion or idea (I think that for now debugging and fixing errors can go first).
Well I tested on Windows maybe the UrlCombobox has an issue on Windows. We will need to check that then
It opened for me the Report error dialog so maybe doing
Not totally sure, I thought there was a signal triggered when a project is closed but maybe I'm wrong (we will need to check) |
I already fix it. Now if a project is not open, the VCS pane is hidden and it shows again if a project is opened.
I remove it in the last commit to remove that annoyance. |
Thanks @trollodel for pushing the fixes, I will test again and check is something is missing or needs to be handled outside the scope of the plugin 👍 |
Oops, I forget to remove workaround code.
Same as above.
I reproduce this issue by spamming the refresh button. This is caused because branches getting is done asynchronously using a QThread.
I never test it but I think it is a git issue, because git creates a branch only if there is at least one commit. I don't know if other VCSs have the same problem. |
61e9a13
to
668c773
Compare
Thanks for the fixes @trollodel . Checking again I think we could improve some more things:
|
@trollodel, we discussed this idea with the rest of the team and agreed that we'd like to have this as a third-party, external plugin (at least for some time). That's because the core of Spyder (i.e. the plugins present in this repo) is already huge (~90 kloc). That way it'd also be easier to report issues (which is what @dalthviz is doing above) and add enhancements, because this repo has too much traffic. Finally, we could also count on you to maintain this plugin (since you'd be the admin of its repo). This is something we've done for other plugins (e.g. spyder-notebook and spyder-terminal) and it's worked well so far. What do you think? |
Are you interested in making it a Spyder dependency? If not, I suggest keeping the VCSBackendBase class (It is almost a list of abstract methods) and the error definitions in the Spyder core to make it part of the API (like the plugins one). This allows core plugins to optionally interact with the VCS plugin as its API is known (that was discussed previously here).
Sure I will! Just, let me know if there is anything that I should know when I maintain it, such as issue management (I see how issues are manage here, so I will try to mimic it), release management, etc.
I keep this PR opened until the repo is created (can you tell me when the repo will be created?) and I continue to fix here @dalthviz reported bugs. |
Yes, our plan is to make Spyder 5 depend on this plugin. Since all our elements (status bar, toolbars, context menus, etc) will be extensible in that version, it should be possible for you to maintain this plugin in an external repo and add any necessary element to the Spyder interface.
Ok, @dalthviz will take care of it.
Sure, no problem. |
Now it should behave in the same way of Projects: showing nothing.
I know. SSH-cloned repo support is completely broken (every interaction with remote using SSH URLs fails). See below.
An empty project is not an empty folder, I cannot know in that point if the remote repository has a
Now it should be fixed.
Good question. On Windows, I assume the default credentials manger (
I now use a set instead of a list to store branches. I hope it will work now. |
Any update about the plugin repository? |
965bb80
to
c2caf83
Compare
With the last commit, I fix several issues left in previous reviews.
Now it tries to set the repository path again. If it fails, an error dialog show and the pane shows the button to create the repo.
This is finally fixed by automatic refresh. But the third column is still not updated.
To prevent flashing and other issues, in many widgets I save the previous backend call result, so only changed objects will be added/removed to the widget. This applies to the branches list too. So duplicated names should never show again.
While I am doing the refractor, the option menu appears again. The issue is caused by me because I previously remove everything in the layout when repo dir changes. |
Minor backend API refractor.
Improve and optimize backend management.
Rename the utils folder to backend. The option button now shows.
Fix discard when the file is not tracked and other bugs
I'm going to close this PR because it's outdated, and it doesn't work in the current master. It received an initial but incomplete review. I understand and agree that there are priorities in the project, but a bit more transparency (or even a simple "We are not interested on this anymore") would save me from waiting for feedbacks that won't arrive. |
I really liked it. Thanks for trying. |
Hi @trollodel, I'm sorry we didn't get to review this one! As you hay have seen, Spyder is undergoing a massive refactor that changes the way about how plugins are created, interact amongst themselves, register items in the main window and how they are destroyed. Since this new model allows for new plugins to have more flexibility, external plugins can now be easily implemented without having to add them to the Spyder core. If you are interested in discussing this, we have a monthly community call where we discuss anything Spyder-related, also, we are planning developer calls starting this or next month to review PRs and efforts like this one. If you are interested in attending this call, please leave us an email or contact to send you the invitation. |
Hi @andfoy. Thanks to response! I'm glad to see that you are still interested in this.
This PR is based on the new Spyder API, and it was developed mainly as an external plugin (the only thing that changes is the plugin registration). > external plugins can now be easily implemented without having to add them to the Spyder core. This was the idea, as described in the following comments: 1, 2, 3. And my replies, with the intention to support and eventually maintain it: 1 2
I'm quite busy this month, so I'm not sure if I can attend the meeting. In any case, this is my email: supertutto01@gmail.com. |
Sorry for dropping the ball with your work @trollodel, but last year was really hard for us in terms of stabilizing our new API and fixing serious performance issues. If you're still interested in pursuing this, we'd be glad to invite you to your organization so you can continue your work here. Otherwise, we'll try to resume it in the next couple of years. |
I thought a bit about this and I have to say that I'm not available to continue the work in the near future. I can help to move this PR to the Spyder organization and make it working again (update the few Spyder API usages). I'm also available to help any contributor that wants to work on this. What do you think? I know I ask you to create a repo that is abandoned since its creation, so I understand that you may refuse the offer. |
I'm ok with your idea and thanks a lot for your offer. I created this repo: https://github.com/spyder-ide/spyder-vcs and gave you Mantain permissions on it. |
@ccordoba12 Thanks for the maintain offer. I already pushed the plugin sources in that repository. In the coming days, I finish the migration, and I'll make the plugin again loadable. It probably needs also some changes to make it in line with other Spyder repos, but I don't want to make pressure for it, so take your time. For anyone following, this PR is now in https://github.com/spyder-ide/spyder-vcs. Anyone interested is welcomed to join us there! |
Thanks @trollodel! Looking forward to see that plugin working again with the latest Spyder 5. |
This PR is now in https://github.com/spyder-ide/spyder-vcs. Anyone interested is welcomed to join us there!
Description of Changes
Before reading
Even though this PR is WIP, I do not push into this PR unless the feature is stable enough.
So, feel free to try it.
For anyone (not necessary Spyder devs) who want to speed up the development of this PR, please read the Contributing section.
Introduction
This PR introduces a dedicated plugin, with its dedicated pane, for VCS management (inspired by Atom's Git integration)
and some APIs that allow to support other VCSs.
The pane adapts itself to the VCS in use (see below for further explanation).
The plugin uses the new Spyder 5 APIs.
Features
Here is the list of implemented VCS features (in the pane).
Implementation structure and design
Both the UI and the backend are designed to be (almost) VCS independent.
The backend essentially consists of implementations of
.utils.api.VCSBackendBase
and a manager for choosing the best implementation (in theory) and instancing it.
The manager is also used as a proxy for the backend instance.
Each implementation does not need to implement the whole API specification,
but it can implement only some of these by decorating each overridden method with
@feature()
.That decorator, without arguments, creates a new enabled feature with the method name as feature name.
This structure allows the UI to create itself and define different behaviors depending on the backend
features, through introspection.
Issue(s) Resolved
Fixes #9001 (only the second and the sixth task)
Fixes #816
Probably require changes in #8415
Limitations and TODO
There are lots of limitation and missing features in this PR.
I tracked them in this issue.
Here I only report the issues that requires changes in the Spyder core (or in another core plugin)
and the issues that requires the approval or suggestion from you, Spyder devs.
Plugin isolation
It keeps its APIs (notably VCSBackendBase) and some common stuffs
in the plugin package.
In addition, it does not have integrations with other Spyder plugins
(except for
sig_project_loaded
that triggers the plugin).Code style and docstring
This is not really an issue, but a question.
I target the code for Python 3.6 (specified in the Spyder's setup.py)
and I use the numpydoc format for the docstrings (but some are too short).
I do not use extra libraries (except for the git that is already required for the existing VCS support).
Are these assumptions correct?
Testing and logging
I know that this PR is not tested at all.
Since I add a dedicated plugin to Spyder, are there some guidelines or suggestion to write test?
Have I the maximum flexibility in writing them?
UI Design
I'm not focusing on the design because I prefer to wait your opinions about it.
Contributing
I opened a dedicated repository
for the development of this PR.
Further information can be found in the repository README.
I do not accept any PR in my fork.
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: trollodel