Add grammar scope to EditorView by thomasjo · Pull Request #2996 · atom/atom

thomasjo · 2014-07-16T20:48:23Z

NOTE: This pull request is still work in progress. I just wanted to open it early to keep my intent and progress public, and discoverable.

The sole purpose of this pull request is to add grammar scope information to EditorView instances, so that e.g. editor key bindings can trigger based on grammar. Useful if you want to invoke different build or formatting commands in various packages, for different types of grammars.

Right now I'm solving this by appending CSS classes to the view, but I'm leaning towards adding a data-scope attribute instead, to prevent pollution and potential conflicts that might cause all manner of mayhem.

thomasjo · 2014-07-16T21:19:21Z

With the change to using data attributes, an entry in keymap.cson might look like this

'.editor[data-scope~="python"]':
  'ctrl-alt-shift-a': 'something:awesome'

and this works even if the data-scope attribute is equal to source python.

thomasjo · 2014-07-16T21:52:08Z

@nathansobo Do the changes to ReactEditorView make sense? It seemed like the best place to add the necessary bits, but I must admit I got a little bit lost trying to figure out the exact hierarchy of the React views and components.

@lee-dohm Was this more or less what you had in mind?

If this seems useful to folks, I'll get create a few tests that cover the changes I've made.

lee-dohm · 2014-07-17T02:10:06Z

Yeah, this seems pretty much like what I was thinking about. Thanks for taking the initiative!

ioquatix · 2014-07-17T02:44:39Z

+1 awesome.

kevinsawicki · 2014-07-21T16:12:18Z

src/editor-view.coffee

What do you think about data-grammar instead of data-scope? It seems a bit more descriptive.

Agreed, and changed!

thomasjo · 2014-07-22T19:48:32Z

Should I go ahead and add a few tests that cover the changes? Extremely busy right now, but I'll find time if necessary

kevinsawicki · 2014-07-28T23:52:09Z

@thomasjo tests would be great whenever you get a chance, also this branch does not appear to cleanly merge anymore so please rebase/merge master.

thomasjo · 2014-08-17T10:12:10Z

@kevinsawicki I've finally gotten around to add a test that verifies The Happy Path™. Let me know if you'd like me to expand with more tests.

I'm unable to actually verify my test though, since I'm still seeing heaps of broken tests on master. And specifically in relation to my test, there are many sibling tests failing because of this error

Error: Must supply an Editor or mini: true
  at EditorView.module.exports.EditorView.initialize (/Users/thomasjo/Projects/atom/src/editor-view.coffee:143:17)
  at EditorView.View (/Users/thomasjo/Projects/atom/node_modules/space-pen/lib/space-pen.js:137:25)
  at new EditorView (/Users/thomasjo/Projects/atom/src/editor-view.coffee:43:3)
  at [object Object].<anonymous> (/Users/thomasjo/Projects/atom/spec/editor-view-spec.coffee:23:24)
  at _fulfilled (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:787:54)
  at self.promiseDispatch.done (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:816:30)
  at Promise.promise.promiseDispatch (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:749:13)
  at /Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:557:44
  at flush (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:108:17)
  at process._tickCallback (node.js:349:11)

Which results in all calls to editorView.attachToDom() to fail because editorView is undefined.

Are the any special magic tricks I need to perform to get all specs to pass locally? Since we don't see the build status in your CI environment, I don't know if they are broken for everyone of if there is merely something wrong with my development environment.

kevinsawicki · 2014-08-19T16:08:20Z

Are the any special magic tricks I need to perform to get all specs to pass locally?

There shouldn't be, you should just be able to put run your spec only using the fdescribe keybinding and then run cmd-alt-ctrl-P to launch the specs.

Since we don't see the build status in your CI environment

Yeah, I really wish we were using Travis for core builds like we do for package builds.

I don't know if they are broken

I ran the spec you added locally and it does fail, I will leave some comments on the lines that need to be changed.

kevinsawicki · 2014-08-19T17:21:05Z

spec/editor-view-spec.coffee

This should be text plain instead of text.plain, and also we usually use toBe instead of toEqual for string comparisons.

Only works when the `grammar-changed` event has been triggered, and only implemented for Space Pen views.

thomasjo · 2014-08-24T12:17:57Z

Adjusted the spec according to your comments, but I'm still unable to run it locally. Still failing with

EditorView
  grammar data attributes
    it adds and updates the grammar data attribute based on the current grammar
      Error: Must supply an Editor or mini: true
        at EditorView.module.exports.EditorView.initialize (/Users/thomasjo/Projects/atom/src/editor-view.coffee:143:17)
        at EditorView.View (/Users/thomasjo/Projects/atom/node_modules/space-pen/lib/space-pen.js:137:25)
        at new EditorView (/Users/thomasjo/Projects/atom/src/editor-view.coffee:43:3)
        at [object Object].<anonymous> (/Users/thomasjo/Projects/atom/spec/editor-view-spec.coffee:23:24)
        at _fulfilled (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:787:54)
        at self.promiseDispatch.done (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:816:30)
        at Promise.promise.promiseDispatch (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:749:13)
        at /Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:557:44
        at flush (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:108:17)
        at process._tickCallback (node.js:349:11)
      TypeError: Cannot read property 'attachToDom' of undefined
        at [object Object].<anonymous> (/Users/thomasjo/Projects/atom/spec/editor-view-spec.coffee:3051:17)
        at _fulfilled (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:787:54)
        at self.promiseDispatch.done (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:816:30)
        at Promise.promise.promiseDispatch (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:749:13)
        at /Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:557:44
        at flush (/Applications/Atom.app/Contents/Resources/app/node_modules/q/q.js:108:17)
        at process._tickCallback (node.js:349:11)


Finished in 1.634 seconds
1 test, 2 assertions, 2 failures, 1253 skipped

Doesn't matter if I run it via Atom GUI or via apm. I believe every single EditorComponent spec fails locally, all seemingly due to the same problem. I'll open an issue for it.

kevinsawicki · 2014-08-25T17:13:48Z

Really not sure why those are failing for you, I've not seen that exception before and the specs are runnable for me locally.

Add grammar scope to EditorView

kevinsawicki · 2014-08-25T17:14:59Z

Thanks for this, it will be included in the next Atom release, 0.124

kevinsawicki reviewed Jul 21, 2014
View reviewed changes

kevinsawicki self-assigned this Jul 22, 2014

kevinsawicki reviewed Aug 19, 2014
View reviewed changes

thomasjo added 7 commits August 24, 2014 10:17

Add grammar scope to EditorView

c6ca03f

Only works when the `grammar-changed` event has been triggered, and only implemented for Space Pen views.

Use data attribute instead of CSS classes

e50887a

Make sure the scope data attribute is always set

6b1868e

Implement support for React Editor

9c154a2

Change attribute name to data-grammar

d3b0654

✅ Add test to verify implementation

dc55d42

🐛 Fix incorrect assertions

4dc89f1

kevinsawicki added a commit that referenced this pull request Aug 25, 2014

Merge pull request #2996 from thomasjo/editorview-grammarscopes

4b8fd22

Add grammar scope to EditorView

kevinsawicki merged commit 4b8fd22 into atom:master Aug 25, 2014

thomasjo deleted the editorview-grammarscopes branch August 26, 2014 18:24

srmibextesting mentioned this pull request Sep 28, 2022

[Snyk] Fix for 11 vulnerabilities srmibextesting/atom#51

Open

This was referenced Oct 5, 2022

[Snyk] Fix for 11 vulnerabilities srmibextesting/atom#57

Open

[Snyk] Fix for 11 vulnerabilities srmibextesting/atom#58

Open

machatman mentioned this pull request Oct 5, 2022

[Snyk] Fix for 12 vulnerabilities machatman/atom#151

Open

NOUIY mentioned this pull request Oct 5, 2022

[Snyk] Fix for 11 vulnerabilities NOUIY/atom#360

Open

srmibextesting mentioned this pull request Oct 6, 2022

[Snyk] Fix for 11 vulnerabilities srmibextesting/atom#62

Open

machatman mentioned this pull request Oct 6, 2022

[Snyk] Fix for 12 vulnerabilities machatman/atom#156

Open

matt-glich mentioned this pull request Oct 6, 2022

[Snyk] Fix for 12 vulnerabilities matt-glich/atom#134

Open

This was referenced Oct 7, 2022

[Snyk] Fix for 11 vulnerabilities srmibextesting/atom#65

Open

[Snyk] Fix for 11 vulnerabilities srmibextesting/atom#66

Open

machatman mentioned this pull request Oct 7, 2022

[Snyk] Fix for 12 vulnerabilities machatman/atom#159

Open

ZachT1711 mentioned this pull request Oct 7, 2022

[Snyk] Fix for 8 vulnerabilities ZachT1711/atom#109

Open

machatman mentioned this pull request Oct 8, 2022

[Snyk] Fix for 12 vulnerabilities machatman/atom#162

Open

ZachT1711 mentioned this pull request Oct 8, 2022

[Snyk] Fix for 8 vulnerabilities ZachT1711/atom#114

Open

snyk-bot mentioned this pull request Oct 9, 2022

[Snyk] Fix for 12 vulnerabilities machatman/atom#164

Open

matt-glich mentioned this pull request Oct 9, 2022

[Snyk] Fix for 12 vulnerabilities matt-glich/atom#138

Open

NOUIY mentioned this pull request Oct 9, 2022

[Snyk] Fix for 11 vulnerabilities NOUIY/atom#364

Open

domsavatta2 mentioned this pull request Oct 23, 2023

[Snyk] Fix for 11 vulnerabilities domsavatta2/atom#4

Merged

Conversation

thomasjo commented Jul 16, 2014

Uh oh!

thomasjo commented Jul 16, 2014

Uh oh!

thomasjo commented Jul 16, 2014

Uh oh!

lee-dohm commented Jul 17, 2014

Uh oh!

ioquatix commented Jul 17, 2014

Uh oh!

kevinsawicki Jul 21, 2014

Choose a reason for hiding this comment

Uh oh!

thomasjo Jul 21, 2014

Choose a reason for hiding this comment

Uh oh!

thomasjo commented Jul 22, 2014

Uh oh!

kevinsawicki commented Jul 28, 2014

Uh oh!

thomasjo commented Aug 17, 2014

Uh oh!

kevinsawicki commented Aug 19, 2014

Uh oh!

kevinsawicki Aug 19, 2014

Choose a reason for hiding this comment

Uh oh!

thomasjo commented Aug 24, 2014

Uh oh!

kevinsawicki commented Aug 25, 2014

Uh oh!

kevinsawicki commented Aug 25, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants