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

Add git to command palette #1243

Merged
merged 12 commits into from
Aug 8, 2023

Conversation

tsabbir96
Copy link
Contributor

@tsabbir96 tsabbir96 commented May 15, 2023

An attempt to develop the feature requested at #1238

@welcome
Copy link

welcome bot commented May 15, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch tsabbir96/jupyterlab-git/add-git-to-command-palette

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

You are on the right path.

In addition to the comments in the code, I must mention that the clone command is defined in a separate plugin; see https://github.com/jupyterlab/jupyterlab-git/blob/master/src/cloneCommand.ts#L20

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated
// Add commands to the pallette
const command = CommandIDs.gitClone;

commands.addCommand(command, {
Copy link
Member

Choose a reason for hiding this comment

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

This will add a new command. But in this specific case we want to add to the palette existing commands. So if you want to test something, you will need to create a dummy command identifier as this one will clash with the existing command).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, thanks for explaining!

src/index.ts Outdated
});
// Add the command to the command palette
const category = 'Git Example';
palette.addItem({ command, category });
Copy link
Member

Choose a reason for hiding this comment

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

This is good. You should do this after line 192 in which addCommands is executed (registering most of this extension commands).

@tsabbir96 tsabbir96 changed the base branch from master to 0.9.x May 16, 2023 07:32
@tsabbir96 tsabbir96 changed the base branch from 0.9.x to master May 16, 2023 07:33
@tsabbir96
Copy link
Contributor Author

tsabbir96 commented May 16, 2023

@fcollonval Hi, I adjusted the changes that you requested. Looks like the PR is failing an Integration Test (UI Test). Here's the generated log:

Running 18 tests using 1 worker
··············F···

  1) tests/merge-commit.spec.ts:59:7 › Merge commit tests › should diff file after clicking ────────

    Error: proxy.waitForSelector: Target closed
    =========================== logs ===========================
    waiting for locator('.jp-git-diff-root') to be visible
    ============================================================

      69 |       .waitFor({ state: 'visible' });
      70 |
    > 71 |     expect(page.waitForSelector('.jp-git-diff-root')).toBeTruthy();
         |                 ^
      72 |   });
      73 |
      74 |   test('should revert merge commit', async ({ page }) => {

        at /home/runner/work/jupyterlab-git/jupyterlab-git/ui-tests/tests/merge-commit.spec.ts:71:[17](https://github.com/jupyterlab/jupyterlab-git/actions/runs/4788641195/jobs/8515773749?pr=1228#step:12:18)

    Test finished within timeout of 60000ms, but tearing down "context" ran out of time.
    Please allow more time for the test, since teardown is attributed towards the test timeout budget.

    attachment #1: video (video/webm) ──────────────────────────────────────────────────────────────
    test-results/tests-merge-commit-Merge-commit-tests-should-diff-file-after-clicking/video.webm
    ────────────────────────────────────────────────────────────────────────────────────────────────

  1 failed
    tests/merge-commit.spec.ts:59:7 › Merge commit tests › should diff file after clicking ─────────
  17 passed (1.8m)
error Command failed with exit code 1.

I would be glad to have your thoughts on this...

@tsabbir96 tsabbir96 requested a review from fcollonval May 18, 2023 05:59
@tsabbir96
Copy link
Contributor Author

Hi @fcollonval sorry to bother you again. Could you provide some insights on this? I am stuck here and any feedback will be much appreciated.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @tsabbir96

Don't worry about the integration tests failing. They are not related to your work.

I only have small comments and this will be ready to go.

src/index.ts Outdated
@@ -185,6 +193,23 @@ async function activate(
gitPlugin.title.icon = gitIcon;
gitPlugin.title.caption = 'Git';

// Add the commands to the command palette
const category = 'Git Operations';
Copy link
Member

Choose a reason for hiding this comment

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

let's internationalize that string:

Suggested change
const category = 'Git Operations';
const category = trans.__('Git Operations');

src/index.ts Outdated
CommandIDs.gitOpenGitignore,
CommandIDs.gitShowDiff,
CommandIDs.gitInit,
CommandIDs.gitClone,
Copy link
Member

Choose a reason for hiding this comment

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

You must not add this one here as it is defined in another plugin.

Suggested change
CommandIDs.gitClone,

Instead you should add the same logic in the clone plugin there:

export const gitCloneCommandPlugin: JupyterFrontEndPlugin<void> = {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You must not add this one here as it is defined in another plugin.

Instead you should add the same logic in the clone plugin there:

export const gitCloneCommandPlugin: JupyterFrontEndPlugin<void> = {

@fcollonval Hi, thanks for the feedback. Could you kindly elaborate a bit for me on what you meant by "Instead you should add the same logic in the clone plugin there". Thank you once again.

Copy link
Member

Choose a reason for hiding this comment

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

gitCloneCommandPlugin is an extension plugin as the plugin you modified there:

const plugin: JupyterFrontEndPlugin<IGitExtension> = {

So you should also add the ICommandPalette token as requirement and then call,

palette.addItem({ command: CommandIDs.gitClone, category: trans.__('Git Operations') })

@fcollonval fcollonval merged commit c639273 into jupyterlab:main Aug 8, 2023
8 checks passed
@welcome
Copy link

welcome bot commented Aug 8, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants