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 activation hooks by grammar scope #17521

Merged
merged 3 commits into from Jun 26, 2018

Conversation

Projects
None yet
3 participants
@Aerijo
Copy link
Member

Aerijo commented Jun 16, 2018

Description of the Change

Adds activation hook for grammar scope name.

Alternate Designs

None

Why Should This Be In Core?

Is core mechanic

Benefits

Makes activation package agnostic, much like the services API.

Possible Drawbacks

None

Verification Process

Tested manually. Will add tests.

Test added.

Applicable Issues

Fixes #14148

@@ -1268,6 +1268,7 @@ module.exports = class Workspace extends Model {

handleGrammarUsed (grammar) {
if (grammar == null) { return }
this.packageManager.triggerActivationHook(`${grammar.scopeName}:root-scope-used`)
return this.packageManager.triggerActivationHook(`${grammar.packageName}:grammar-used`)

This comment has been minimized.

@thomasjo

thomasjo Jun 16, 2018

Member

Outside the scope of this PR, but remarking on it anyway; should we :fire the return here? There is no value returned by triggerActivationHook, so returning here doesn't make any sense and is simply confusing.

This comment has been minimized.

@Aerijo

Aerijo Jun 16, 2018

Author Member

I'd remove it. Should be fine to do it in this PR right?

This comment has been minimized.

@thomasjo

thomasjo Jun 16, 2018

Member

Yeah, definitely fine in my opinion. Just didn't want to expand the scope of your PR 😅

@Aerijo

This comment has been minimized.

Copy link
Member Author

Aerijo commented Jun 16, 2018

The test is just a copy of the one above, but the hook changed to source.js:root-scope-used etc.

I think it passes, but running it on my computer seemed to always cause a timeout on the promises (for this, the one above, and several others that changed on each run).

@thomasjo
Copy link
Member

thomasjo left a comment

LGTM 🚢

@daviwil
Copy link
Member

daviwil left a comment

Thanks a lot @Aerijo, looks great!

@daviwil daviwil merged commit d82c397 into atom:master Jun 26, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.