-
Notifications
You must be signed in to change notification settings - Fork 326
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
Add git stash #1228
Add git stash #1228
Conversation
Hi @fcollonval, I am running into this assertion error. For some reason, my response body has the environment attached to it as well. Do you know how I might fix this error? I have added GitStashHandler, Git.stash, and test_stash test_stash.py
Error when I run the test for `test_stash.py`: looks like the environment is being passed too?
|
Nevermind, I figured it out with @basokant! I was just missing some parameters. I'm unblocked now. |
Upon writing some more tests, I ran into this block of code. I think I'm getting the hang of using it but I'm confused upon reading the jp_fetch documentation. For example, when calling jp_fetch, we pass some parameters such as the git commands (stash) or in the test_remote example (remote, add). However, upon looking into the jp_fetch fixture, the parameters look different. Why is this? Using jp_fetch
Definition of jp_fetch
|
Just an update to this issue. I was able to successfully create a button that calls I currently re-used the Pull Button component badge/action button as a placeholder. I'm not sure if I should create a new button for stash. I still need to
|
Hey @shawnesquivel The concept of fixtures in Pytest can be confusing. Taking the example from the link above: import pytest
class Fruit:
def __init__(self, name):
self.name = name
def __eq__(self, other):
return self.name == other.name
@pytest.fixture
def my_fruit():
return Fruit("apple")
@pytest.fixture
def fruit_basket(my_fruit):
return [Fruit("banana"), my_fruit]
def test_my_fruit_in_basket(my_fruit, fruit_basket):
assert my_fruit in fruit_basket What pytest is doing with that code:
So in regular Python way, the test call is actually: fruit = my_fruit()
basket = fruit_basket(fruit)
test_my_fruit_in_basket(fruit, basket) Note that the returned value can be anything. In particular for import pytest
@pytest.fixture
def my_fruit():
return "banana"
@pytest.fixture
def cook_with(my_fruit):
def cook(*ingredients):
return f"Delicious food combining {my_fruit} and {', '.join(ingredients)}"
return cook
def test_my_fruit_in_basket(my_fruit, cook_with):
assert my_fruit in cook_with("sugar", "butter") The For |
Some pair programming with @basokant and we got further! 😄 Update:
Next Steps:
|
Hi @fcollonval , Question 1:
Can you provide some guidance as to how I can do step 2? I had an idea to use:
Question 2: Question 3: Question 4: How can I make a dialogue box that allows the user to enter the stash message when they click the stash button? |
Great work 🚀
I'm not sure to correctly understand the question. Do you want to update the file status in If you want to update the file status, you can call Line 1013 in 91c3073
Waiting for more info see Q1
Usually this is done by listening to a signal emitting model changes. See for multiple examples: jupyterlab-git/src/components/GitPanel.tsx Line 190 in 91c3073
You can use a React component
You can make use of the JupyterLab helper |
Thank you for the feedback! Related to q1 - on VS code, when I click git.stash.-.vs.code.movCurrently, when I call the git.stash.-.jlg.blocker.mov |
Forgot to tag you on the previous reply – @fcollonval |
For Q3, it is related to my previous question, I want the entire file to refresh so that the stash changes will be removed (if I stash) or re-applied (if I apply/drop). So I'm not sure where to emit the signal from so that I can achieve this. |
Hi @fcollonval, I worked on it a little more and I think I have a new issue that I don't really understand. It may make my previous questions a little ignorant of the bigger issue. Please see the video below: When I call the git.stash.issue.march.23.2023.mov |
Hey @shawnesquivel, To force reloading a file for it to match the disk content, you can use the Line 1759 in 91c3073
When you do modification outside the UI, there is no way for the UI to know a modification happens. The file status is updated because it is polled regularly - you could do that for stash list to see it appearing. But for the file content, there is no mechanism. At best, JupyterLab should tell you the next time you try to save the file that it has been modified. |
Does that help you to move forward? |
I get the same issue when I click the stash button manually (inside the UI - see video). So if I wanted to revert all the files affected, I could just pass each file that was changed into the revertFile? git.stash.ui.issue.-.march.24.2023.mov |
Hey @fcollonval , forgot to tag you in the response above again, but hopefully it notified you. |
Yes this should do the trick |
Hi @fcollonval , I made it a little prettier 😄 Please see the top post for the latest demo/UI. Let me know what you think. i'll move on to finishing the tests and starting playwright otherwise. Also, is there a way to check the Build tests in the VS code without pushing? Lots of them are "prettier" issues. I'm not sure if it's something I can fix. |
👍
You can run locally all or some of the following commands:
|
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 @shawnesquivel
Here is a first large review of your code. Overall the code looks code.
src/model.ts
Outdated
await this.refreshStash(); | ||
|
||
if (this._stash?.length > 0) { | ||
this._stash[0].files.forEach(file => { |
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.
could you get the list before calling the task handler? The trouble is that it can take time to refresh the stash and we should revert the file as quickly as possible.
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.
Doesn't this create an issue? If we revert the files before stashing, then we may stash the reverted file, thereby losing the changes that we wanted to stash..
try { | ||
|
||
// Grab the list | ||
const stashFiles = this._stash[index].files; |
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.
What happen if index
is undefined?
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.
Good point. I changed it so that it will apply the first stash if no index is provided.
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 also changed the index from an optional argument to required.
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 @shawnesquivel
Here is a first large review of your code. Overall the code looks code.
Thanks @fcollonval. I just finished going through all the PR comments and have some feedback and follow up questions that I left in the resolved threads. |
Hi @fcollonval, I'm getting some build errors: |
Hi @fcollonval , I'm getting some build errors. Could you provide any suggestions to fix them? Build Error 1: I'm getting this signal connection issue, even though my signal does connect regularly when I run Jupyter Lab. Signal connects normally here:
Error Messages: Test failing with signal connect● GitPanel › #render() › should render Commit and Push if there is a remote branch
● GitPanel › #render() › should render Commit if there is no remote branch
● GitPanel › #render() › should render Commit if there is a remote branch but commitAndPush is false
Build Error 2 - Fail to load server extension Running Running Error messagesonsole.error Failed to load the jupyterlab-git server extension settings Error: The versions of the JupyterLab Git server frontend and backend do not match. The @jupyterlab/git frontend extension has version: 0.41.0 while the python package has version 0.1.0. Please install identical version of jupyterlab-git Python package and the @jupyterlab/git extension. Try running: pip install --upgrade jupyterlab-git at Object.activate (/Users/shawnesquivel/GitHub/jupyterlab-git/src/index.ts:116:13) at processTicksAndRejections (node:internal/process/task_queues:95:5) at Object. (/Users/shawnesquivel/GitHub/jupyterlab-git/tests/plugin.spec.ts:128:25)
run jupyter server extension listupyter_server_fileid enabled |
Hi @fcollonval, related to the playwright installation, I'm facing an error. Following the instructions outlined here: https://github.com/jupyterlab/jupyterlab-git/tree/master/ui-tests Steps 0, 1, and 2 work. When I run step 3 jlpm install - error messageyarn install v1.21.1 |
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 a lot @shawnesquivel the Python code looks great and the TypeScript code is quite good. I have a couple of comments and regarding the trouble with canvas, could you try rebasing this PR on master. I think you will benefit from the fixes merged last week.
Hi @fcollonval, as mentioned in our last meeting, these pytests are not passing. They don't seem to be related to my changes, so it would be great if you could take a look before merging. However, I'm still writing the playwright tests.
|
Hi @fcollonval ! Tests Failing to Update UI After Running Build
jupyter lab --config ./ui-tests/jupyter_server_test_config.py jlpm playwright test git-stash.spec.ts
- So the question is, why isn’t the test showing the latest UI updates even after running Steps to Troubleshoot
|
@shawnesquivel could it be that if the side panel width is to small action buttons get out of the viewport (you could test that locally by resizing the panel to be very small). If this is the case, try updating the CSS rules on the stash description to have it display an ellipsis when overflow occurs. If not let me know I'll try to have a look locally. |
The skipped ones are because you do not have some optional dependencies installed. Don't worry the CI gets you covered. For the two failing tests, it could be interesting to get more information about the error. But my guess is that it is related to the OS. As they are working on the CI and it is not related as you mentioned, let's move forward. |
Hi @fcollonval ! I worked with @basokant to get some more integration tests done. I also ended up finishing the rest (I think)! Some of the methods are a little hacky, I used "timeout" since there were multiple waiting dependencies (wait for revertFile, wait for stash list to re-update). I think I could also clean up some of the expect methods too, since I think the The tests seem to have passed in my local env, but something is going wrong when pushing to GitHub. Let me know what the last steps should be to get this fixed up before the end of the fellowship 😄 |
HI @fcollonval , I added the changes mentioned in today's meeting. All my tests are passing now! Looks like the test failing is not related to this issue?
|
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 a lot @shawnesquivel
Overall it looks very good. Would you be able to finish it up? Otherwise I'm happy to finish the PR.
You are correct the integration failure is not related.
Hi @fcollonval , I added the final changes you requested! Let me know if there's anything else. |
Kicking the CI |
Add #309 to use git stash
add.git.stash.-.apr.3.2023.mov