Add "Line Based" support #112

erran · 2014-04-22T07:01:48Z

Add keycaps for "Line Based" runs
Add menus for "Line Based" runs
- Right/ctrl clicking the line number will provide a menu option to run at the current line.
Update grammars.coffee for "Line Based" runs

rgbkrk · 2014-04-22T14:12:27Z

keymaps/script.cson

@@ -6,4 +6,5 @@
  'cmd-i': 'script:run'
  'ctrl-w': 'script:close-view'
  'ctrl-c': 'script:kill-process'
+  'shift-cmd-j': 'script:run-at-line'


I would have used shift-cmd-l, but this is fine.

@rgbkrk I was actually thinking the same. I have fat fingered cmd-j a few times which causes Atom to join lines (remove the previous line's new line char).

@rgbkrk Just realized that shift-cmd-L is used by Core. I stripped this change.

rgbkrk · 2014-04-22T14:21:56Z

I'm not a big fan of this argument explosion.

A little bird tells me we should create a CodeContext object that has getPath(), getLineNumber(), etc. methods.

erran · 2014-04-22T16:45:29Z

@rgbkrk Sounds good. I'll do that tonight.

It's worth mentioning that when selecting the entire line the next line is the number that is used. From memory of my earlier implementation: checking for the current selection and grabbing Math.min from the bufferRowRange would give the correct line number in the case of a whole line selection.

rgbkrk · 2014-04-29T15:31:14Z

Sorry I had not come back to this for a bit. I definitely appreciate the CodeContext abstraction and that this will work well across line, selection, and file based runs. 😄

erran · 2014-04-30T04:26:35Z

I'm not a big fan of getters if they're just returning an attribute. I'd rather just use the attribute in those cases.

However - this is probably where we can address #86.

@rgbkrk If we do we'd have to send codeContext vs. codeContext.getCode. Perhaps send both context and the actual code as the function arguments?

erran · 2014-04-30T04:29:53Z

Can you add tests for the codeContext?

I'm not sure I follow do you mean in specs or to check that it's set. If the latter, where would I check since it's set outside of any conditionals.

rgbkrk · 2014-05-05T19:41:38Z

I mean adding specs to this PR, so we can actually have unit tests for our own sanity.

See merge-conflicts for some great examples.

erran · 2014-05-06T04:10:21Z

@rgbkrk you got it. I'll try to get to this tomorrow night or Wednesday some time.

rgbkrk · 2014-05-25T18:08:32Z

Still in the mood for some tests? 😉

erran · 2014-05-25T21:19:28Z

I've been on vacation for the last three weeks. I'll try to get to it this week, assuming no urgent things popped up at work, though.

rgbkrk · 2014-05-25T21:21:14Z

Hey I understand that. Hope vacation was great!

erran · 2014-05-30T04:40:47Z

Started with something. See this commit 82eb5e4.
Is that what you had in mind? I'll get more done tomorrow.

rgbkrk · 2014-05-30T04:43:02Z

Most definitely. I think you can clean out those junk tests that were basically boilerplate at this point.

erran · 2014-05-30T04:53:04Z

@rgbrkrk I ended up pushing what I had for the night. Turns out I can't find my Mac charger at home since my vacation, so I'll have to bring the spare from the office tomorrow. 😩

rgbkrk · 2014-06-01T01:55:14Z

Ok, finally had a chance to look over this again with the new CodeContext object. I was really hoping that we would just use the CodeContext piece within all the grammars instead of casing off which argument is being run (as well as introducing that new parameter).

rgbkrk · 2014-06-01T01:56:55Z

lib/grammars.coffee

@@ -130,6 +136,9 @@ module.exports =
    "File Based":
      command: "rspec"
      args: (filename) -> ['--tty', '--color', filename]


I'm expecting us to use the CodeContext objects here too, instead of the filename as the argument.

Great! 😄 Not sure what made me change this (or abstain from doing so).

rgbkrk · 2014-06-01T03:41:39Z

Not surprisingly, since I just merged in Yet Another Language™ you'll need to fetch and rebase against upstream/master (or whatever remote name you have configured for github.com/rgbkrk/atom-script).

git fetch upstream
git rebase -i upstream/master

+ Add keymaps for "Line Based" runs + Add menus for "Line Based" runs + Update grammars.coffee for "Line Based" runs

erran · 2014-06-01T03:50:47Z

@rgbkrk I had less merge conflicts than I'd have guessed. 😉

erran · 2014-06-01T03:53:11Z

@rgbkrk Going to sort out the specs now. Let me know if you have any other things to change in mind.

rgbkrk · 2014-06-01T04:06:11Z

lib/script-view.coffee

-      else
-         arg = selection.getText()
+
+    console.log "#{argType} executing"


Was this meant as a debug statement?

+ Update related specs

rgbkrk · 2014-06-01T04:32:44Z

I'm currently trying to see if there's a good way to abstract this a bit more. Having setup be the main toggle lacks a lot of readibility. I kind of wish there were multiple entry points (in function name only), like runLine and run for the outer calls. It would be nice if setup was instead called createCodeContext and that we just encapsulated the overall run in that one code context object. Hmmm...

erran · 2014-06-01T04:41:13Z

@rgbkrk setup lines 74 through 97 could probably be moved into a validateLang() method for starters. That'd reduce a lot of code that'd need to be duplicated.

   # Get language
   lang = @getLang editor
   err = null

  # Determine if no language is selected.
   if lang is 'Null Grammar' or lang is 'Plain Text'
     err = $$ ->
       @p 'You must select a language in the lower left, or save the file
         with an appropriate extension.'

   # Provide them a dialog to submit an issue on GH, prepopulated with their
   # language of choice.
   else if not (lang of grammarMap)
     err = $$ ->
       @p class: 'block', "Command not configured for #{lang}!"
       @p class: 'block', =>
         @text 'Add an '
         @a href: "https://github.com/rgbkrk/atom-script/issues/\
           new?title=Add%20support%20for%20#{lang}", 'issue on GitHub'
         @text ' or send your own Pull Request.'

   if err?
     @handleError(err)
     return false

rgbkrk · 2014-06-01T04:41:39Z

Yeah, gimme a sec. I'll probably end up submitting a PR against your branch shortly.

erran · 2014-06-01T04:42:25Z

@rgbkrk Fine with me. 👍

* Move CodeContext creation into its own build step * Move language validation to its own function * Finish extracting lang, codeContext * Simplify line based path * Separate line and file/selection runs * Simplify code context selection

Move code context creation

erran · 2014-06-10T05:21:46Z

Playing with a fix for #86 I ended up with this.

EDIT: Just realized that my change currently subtracts an extra line if we're going by a partial selection vs. an entire line. Which should we choose as the default? It's a matter of the shortcuts or no. of clicks a user uses for their selection.

erran · 2014-06-10T05:38:34Z

My second shot on a fix for #86

erran · 2014-06-10T06:24:14Z

Here's the gif I'm thinking of replacing the README gif with:

rgbkrk · 2014-06-10T06:32:49Z

Love your color scheme, love the new README GIF, glad you showed off the right-click to run at line.

rgbkrk · 2014-06-10T06:35:45Z

spec/code-context-spec.coffee

+      code = @codeContext.getCode()
+
+      expect(typeof code).toEqual('string')
+      expect(code).toMatch("print 'hello world!'")


Must say, thanks again for adding the tests.

No problem!

rgbkrk · 2014-06-10T06:37:22Z

Once you add the comment about why the whitespace was added for Python runs (or addressed for more langs), we . Thanks for the new Line Based functionality AND the refactor to have CodeContext objects.

erran · 2014-06-10T06:38:13Z

I think I'll remove that commit for the sake of this PR since the proper fix would be multi-lingual.

rgbkrk · 2014-06-10T06:40:43Z

I think I'll remove that commit for the sake of this PR since the proper fix would be multi-lingual.

I was about to say the same thing. Let's hash that out in a new PR.

erran · 2014-06-10T06:43:51Z

@rgbkrk Removed the commit. I'll add a new PR soon.

Add "Line Based" support

rgbkrk · 2014-06-10T06:53:57Z

I'll make the release and update the relevant bits tomorrow morning. For now, I'm going to 😴. If you get the whitespace fix in the next day, we'll add that in as part of the release.

erran changed the title ~~Add "Selection Based" support~~ Add "Line Based" support Apr 22, 2014

erran mentioned this pull request Apr 22, 2014

Support "Line Number Based" runs #109

Closed

rgbkrk reviewed Apr 22, 2014
View reviewed changes

rgbkrk reviewed Jun 1, 2014
View reviewed changes

erran added 7 commits May 31, 2014 22:42

Add "Line Based" support

b0fd9f6

+ Add keymaps for "Line Based" runs + Add menus for "Line Based" runs + Update grammars.coffee for "Line Based" runs

Create a CodeContext object to wrap some logic

6fc4ba0

Use CodeContext#filepath not CodeContext#getPath

d71ac41

Create spec/code-context-spec.coffee

66ef769

Delete boiler plate spec file

6689e90

Always send codeContext as an argument to buildArgs

8a7e009

Update header-view title for line based runs

7fcfcb2

rgbkrk reviewed Jun 1, 2014
View reviewed changes

Fix fileColonLine behavior

89ee133

+ Update related specs

Add a new spec for testing getCode()

448a7ed

rgbkrk mentioned this pull request Jun 1, 2014

Move code context creation erran/atom-script#1

Merged

rgbkrk and others added 2 commits June 8, 2014 21:01

Reorganize code layout

5def092

* Move CodeContext creation into its own build step * Move language validation to its own function * Finish extracting lang, codeContext * Simplify line based path * Separate line and file/selection runs * Simplify code context selection

Merge pull request #1 from rgbkrk/move_code_context_creation

8eb428d

Move code context creation

Add lineNumber to Selection Based runs

cca801e

rgbkrk reviewed Jun 10, 2014
View reviewed changes

Update demo gif in README.md

cb4e1a7

rgbkrk added a commit that referenced this pull request Jun 10, 2014

Merge pull request #112 from erran/line-based-execution

425440d

Add "Line Based" support

rgbkrk merged commit 425440d into atom-community:master Jun 10, 2014

erran deleted the line-based-execution branch June 10, 2014 07:17

Add "Line Based" support #112

Add "Line Based" support #112

Conversation

erran commented Apr 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgbkrk commented Apr 22, 2014

erran commented Apr 22, 2014

rgbkrk commented Apr 29, 2014

erran commented Apr 30, 2014

erran commented Apr 30, 2014

rgbkrk commented May 5, 2014

erran commented May 6, 2014

rgbkrk commented May 25, 2014

erran commented May 25, 2014

rgbkrk commented May 25, 2014

erran commented May 30, 2014

rgbkrk commented May 30, 2014

erran commented May 30, 2014

rgbkrk commented Jun 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgbkrk commented Jun 1, 2014

erran commented Jun 1, 2014

erran commented Jun 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgbkrk commented Jun 1, 2014

erran commented Jun 1, 2014

rgbkrk commented Jun 1, 2014

erran commented Jun 1, 2014

erran commented Jun 10, 2014

erran commented Jun 10, 2014

erran commented Jun 10, 2014

rgbkrk commented Jun 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgbkrk commented Jun 10, 2014

erran commented Jun 10, 2014

rgbkrk commented Jun 10, 2014

erran commented Jun 10, 2014

rgbkrk commented Jun 10, 2014