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

support workspaceContains activation events #5649

Merged
merged 1 commit into from
Jul 12, 2019

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Jul 5, 2019

What it does

How to test

See how to install VS Code extensions: https://github.com/theia-ide/theia/wiki/Testing-VS-Code-Extensions

Review checklist

  • as an author, I have thoroughly tested my changes and carefully reviewed in accordance with the review guideline

Reminder for reviewers

  • each approve has supporting comments in accordance with the review guideline
  • the approve without a comment should be dismissed

@akosyakov akosyakov force-pushed the ak/complete_activation_events branch 2 times, most recently from b7b20a1 to b188602 Compare July 5, 2019 15:14
@akosyakov
Copy link
Member Author

akosyakov commented Jul 5, 2019

It is ready for the review. You can also try rust extension, all activation events for it should be supported now.

@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Jul 5, 2019
@akosyakov
Copy link
Member Author

@BroKun if you want to get more involved feel free to review PRs as well. Community help is always welcomed in any form.

@kittaakos
Copy link
Contributor

I am reviewing this...

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

It worked for me, I could see the expected two plug-in activations.
It did not happen when I tried with another repository than Theia.

@akosyakov akosyakov force-pushed the ak/complete_activation_events branch from b188602 to 99e242a Compare July 11, 2019 10:33
@akosyakov
Copy link
Member Author

@kittaakos I've addressed your comments, please have a look again.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov force-pushed the ak/complete_activation_events branch from 99e242a to 1fc845d Compare July 11, 2019 10:34
@kittaakos
Copy link
Contributor

please have a look again.

👍

I have tried it again from a fresh clone, copied the two plugins to the plugins folder:

  • I could see the two notification after opening a workspace on theia.
  • I did not experience any plugin activations when I opened a workspace on a different repository.

@akosyakov akosyakov added the CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) label Jul 12, 2019
@akosyakov
Copy link
Member Author

@marcdumais-work I need it to resolve #5661 properly. Can we proceed with merging? a CQ for 5 lines from VS Code repo with MIT license

@marcdumais-work
Copy link
Contributor

@marcdumais-work I need it to resolve #5661 properly. Can we proceed with merging? a CQ for 5 lines from VS Code repo with MIT license

Let me have a look...

@marcdumais-work
Copy link
Contributor

@akosyakov It seems to me that the CQ in this specific case is a formality:

  • MIT license is "compatible"
  • this area of the code looks to be 100% original Microsoft code
  • you added the original Microsoft license header

So I am personally ok to merge without waiting, if we accept a small risk that we may have missed something and, in that case, we might need to go back and potentially roll-back this PR.

Also, we will need the CQ approved before the next release, so we are "license certified".

@akosyakov
Copy link
Member Author

a CQ was approved, merging

@akosyakov akosyakov merged commit 4535f24 into master Jul 12, 2019
@akosyakov akosyakov deleted the ak/complete_activation_events branch July 12, 2019 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants