-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[plugin] Add PHP grammar contributions #4582 #4772
[plugin] Add PHP grammar contributions #4582 #4772
Conversation
@@ -0,0 +1,93 @@ | |||
/******************************************************************************** | |||
* Copyright (C) 2018 Red Hat, Inc. and others. |
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.
it's 2019
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.
yes, sorry. I'll add a fixup
readonly id = 'php'; | ||
readonly scopeName = 'text.html.php'; | ||
|
||
readonly config: monaco.languages.LanguageConfiguration = { |
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.
if it's copied from MS repo, please attribute copyrights properly
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.
Technically it's not.
File packages/textmate-grammars/src/browser/php.ts, as well as this one, are the modified copies of https://github.com/theia-ide/theia-php-extension/blob/master/php/src/browser/php-grammar-contribution.ts, not directly copied MS. So, the copyright here probably should be of 'TypeFox' (if not the 'RedHat').
The only doubt I have here is that probably we souldn't have it in this separate files - probably it's better to combine registering the language and HTML PHP grammar inside packages/textmate-grammars/src/browser/php.ts if possible (similar to how it's done in MS's https://github.com/Microsoft/vscode/blob/master/extensions/php/package.json#L32).
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.
The only doubt I have here...
i see, php language config should be registered only once in PhpGrammarContribution
, no need to register it again in PhpHtmlGrammarContribution
.
} | ||
}); | ||
|
||
registry.mapLanguageIdToTextmateGrammar(this.id, this.scopeName); |
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 don't thin we support multiple scopes per a language. It looks like we need to drop phphtml support till it is implemented. Could you file an issue to support multiple scopes?
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.
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.
OK. I'll remove phphtml for now and open an issue to support multiple scopes
@akosyakov |
@tolusha Type B should be fine as far as I know cc @marcdumais-work |
The issue #4796 is opened for multiple contexts per a language support |
There should a PR against https://github.com/theia-ide/theia-php-extension which removes textmate grammars. It should be merged immediately after this PR is merged to avoid breaking clients using next. Code looks good now in this PR. |
@akosyakov Then I'm going to prepare a PR for https://github.com/theia-ide/theia-php-extension before we merge this one |
The following PR is added in order to remove PHP grammar contribution from |
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.
thank you
@@ -0,0 +1,189 @@ | |||
{ |
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.
It does not seem to be used, please remove if it not
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.
But then, when #4796 is resolved, we'll have to add it again. Are your sure we must remove it for now?
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.
There should a PR against https://github.com/theia-ide/theia-php-extension which removes textmate grammars. It should be merged immediately after this PR is merged to avoid breaking clients using next.
Code looks good now in this PR.
@akosyakov
I can merge this PR, but it looks like I'm not allowed to merge complimentary grammars removal PR theia-ide/theia-php-extension#17
Could you, please, merge that complimentary PR once we've merged this one?
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.
sure i can do, just ping me there after merging
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.
But then, when #4796 is resolved, we'll have to add it again. Are your sure we must remove it for now?
you can comment on the issue that it should be added after the PR is resolved, right now i don't see a reason to commit it (sorry missed this comment first time)
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.
OK. Then I'm removing it from the current PR.
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've removed packages/textmate-grammars/data/php.html.tmLanguage.json from the PR. So it looks like we only need to wait for the CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19509 to be resolved
@akosyakov I've squashed the fixup in PR and rebased it against current master. (however currently unused packages/textmate-grammars/data/php.html.tmLanguage.json is still not excluded from the PR) |
Hi @tolusha
For cope we copy from a non-eclipse project and (potentially) modify, I think we need a "Type B" CQ. So I do not think the CQ you opened is valid for this case. On the first CQ submission page, you should chose: More detailed instructions for this case are here |
@tolusha, @marcdumais-work , @akosyakov Yet another CQ is opened for the issue, so we need to wait again for it to be resolved. |
@vrubezhny please link a CQ |
@akosyakov Sorry, I forgot the link: |
@vrubezhny CQ is good: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19509#c3 please rebase and merge |
The CQ [1] needs to be resolved before merging this commit Need to remove the similar contribution from PHP extension[2] [1] https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19282 [2] https://github.com/theia-ide/theia-php-extension Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>
The CQs [1], [2] are resolved, so it's possible to contribute PHP grammars
Need to remove the similar contribution from PHP extension[3]
[1] https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19282
[2] https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19331
[3] https://github.com/theia-ide/theia-php-extension
Signed-off-by: Victor Rubezhny vrubezhny@redhat.com