Support JS package generation #36

stevenhauser · 2015-11-22T19:52:57Z

Addresses #14

Now that atom/apm#417 has been merged, JS package generation can be done from the UI, so that's what happens here. I ran with the third idea I mentioned in #14 because it made the most sense to me.

stevenhauser · 2015-11-22T19:55:39Z

The code is pretty simple here and all specs pass, but I had problems using my version of apm with atom/apm#417 in it, so I wasn't able to 100% 🌽firm that this works. Going to look into it further, but any tips on using an apm outside of Atom.app/Contents/Resources/app/apm/node_modules/.bin/apm? Every time I change the symlink, Atom seems to change it back to that.

stevenhauser · 2015-11-22T20:04:53Z

Update on actual testing of this feature: since initPackage uses atom.packages.getApmPath() and that uses atom.packages.apmPath (at least on OSX), I just set that to my local apm path in the dev tools 🌽sole to test this. It worked as expected. Let me know if there's a better way.

50Wliu · 2015-11-22T21:57:48Z

lib/package-generator-view.coffee

@@ -89,6 +97,9 @@ class PackageGeneratorView extends View

    @runCommand(atom.packages.getApmPath(), args, callback)

+  isInPackageMode: ->


Is this necessary? if @mode is 'package' seems fine to me, and it's also similar to what vim-mode does.

Yeah that's what it was here but since we're doing the same check in two places now, a method for the check seems better than a constant or string duplication to me.

50Wliu · 2015-11-22T21:58:48Z

spec/package-generator-spec.coffee

+            expect(apmExecute.argsForCall[0][1]).toEqual packageInitCommandFor "#{packagePath}", 'coffeescript'
+
+      describe "when the package is a javascript package", ->
+        it "calls `apm init`  with the correct syntax option", ->


extra space after apm init

50Wliu · 2015-11-22T21:59:15Z

spec/package-generator-spec.coffee


    describe 'when creating a package', ->
+


This newline isn't needed

stevenhauser · 2015-11-22T22:24:41Z

@50Wliu Is there a preference for separate commits for fixing things like spacing issues vs squashing and force pushing? I tend to do the latter but know that can be a PITA for collaboration.

50Wliu · 2015-11-22T22:59:14Z

@stevenhauser I don't think the Atom team has a definitive preference, but maybe a slight lean towards not squashing.

vinkla · 2015-12-02T12:14:19Z

👍

stevenhauser · 2016-03-04T13:17:05Z

Hey it's been a few months—is there still interest in this? There was some conversation in #14 about different ways to approach this, but I think it'd be nice to go this simple route to at least have something available and we could enhance it later? Happy to update this and fix any conflicts.

MadLittleMods · 2016-03-04T13:32:53Z

@stevenhauser I am still interested in having this available 😀! The package syntax option seems good to me.

Source changes: - Add `isInPackageMode` method to minimize string comparisons of `@mode` - Add `getInitOptions` method to facilitate differentiating options between modes and to keep `initPackage` simple. Add `--syntax` option here, too. Spec changes: - Add `packageInitCommand` helper to clean up init command test argument duplication and to prevent it from getting even longer with the new `--syntax` addition. - Move `generateOutside` into more generic `generatePackage` to clean up duplication b/w the inside/outside specs, and leverage `_.partial` to create more readable helper functions. This is also to prevent further duplication in the new syntax specs. - Write specs to check `--syntax` option.

stevenhauser · 2016-03-06T17:30:23Z

@lee-dohm I've resolved the merge conflicts here. The problem was that the configuration was moved out of main.coffee into package.json, so I moved this new config option in there as well.

However I'm having problems actually getting this to load into Atom that I didn't have before. Here are the steps I've taken:

Clone my fork
npm install within fork directory
apm link within fork directory
atom . or atom . --dev within fork directory

When I first opened this PR, I believe that step 4 loaded my dev version of package-generator in as expected, but that's not happening for me anymore. Has something changed where I need to do another step or something?

lee-dohm · 2016-03-06T18:02:15Z

No, you shouldn't need to do another step, though I would recommend revising your steps to this:

Clone
apm install (there are subtle differences between how apm and npm work and apm will do the installation of any Node dependencies)
apm link --dev
atom --dev . (not sure the order of the parameters matters, just being pedantic 😀)

stevenhauser · 2016-03-06T22:15:32Z

Thanks for the tip @lee-dohm but that didn't work either. I think something might be messed up on my machine due to multiple user accounts or something because I'm still stuck on Atom 1.3.3 and updates won't install. Regardless, I still think this PR is good to review because it was fine before and the only real change I made recently was to move config to package.json.

lee-dohm · 2016-03-07T20:22:59Z

I've added it to the queue for review. We're still trying to catch up on the backlog, so I can't give you an ETA. Just know that we're working on it 😀

stevenhauser · 2016-03-07T23:04:54Z

Coolio, I'm in no rush. Thanks!

joshaber · 2016-03-14T19:40:39Z

package.json

@@ -28,6 +28,12 @@
      "default": false,
      "type": "boolean",
      "description": "When disabled, generated packages are linked into Atom in both normal mode and dev mode. When enabled, generated packages are linked into Atom only in dev mode."
+    },
+    "packageSyntax": {
+      "default": "coffeescript",


I think we should default to JavaScript since it's the future.

Hey that's great, I defaulted to CS purely for the sake of "backwards compatibility." Changing.

That's almost always the right choice, but if we can push people away from CS and to JS... 😄

joshaber · 2016-03-14T19:50:36Z

@stevenhauser This looks good! Just one comment.

stevenhauser · 2016-03-15T00:07:56Z

Updated @joshaber. Thanks for the review!

joshaber · 2016-03-15T14:32:28Z

Awesome, thanks @stevenhauser!

Support JS package generation

50Wliu added the needs-review label Nov 22, 2015

50Wliu reviewed Nov 22, 2015
View changes

spec/package-generator-spec.coffee Outdated

describe 'when creating a package', ->

This comment has been minimized.

Sign in to view

50Wliu Nov 22, 2015
Member

This newline isn't needed

stevenhauser mentioned this pull request Nov 23, 2015

Allow generation of JavaScript #14

Closed

lee-dohm added the atom label Mar 4, 2016

stevenhauser added 2 commits Mar 6, 2016

Add packageSyntax configuration option

6c585c4

joshaber self-assigned this Mar 14, 2016

joshaber reviewed Mar 14, 2016
View changes

🎉 Switch default package syntax from CS to JS

Loading status checks…

5134396

joshaber added a commit that referenced this pull request Mar 15, 2016

Merge pull request #36 from stevenhauser/support-js-package-generation

Loading status checks…

6d7238d

Support JS package generation

joshaber merged commit 6d7238d into atom:master Mar 15, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

thomasjo removed the needs-review label Mar 16, 2016

Hammster mentioned this pull request Jul 12, 2016

word-count example is a little bit outdated atom/flight-manual.atom.io#225

Closed

Nov	DEC	Jan
	30
2019	2020	2021

atom / package-generator

Support JS package generation #36

Support JS package generation #36

stevenhauser commented Nov 22, 2015

stevenhauser commented Nov 22, 2015

stevenhauser commented Nov 22, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

stevenhauser commented Nov 22, 2015

50Wliu commented Nov 22, 2015

vinkla commented Dec 2, 2015

stevenhauser commented Mar 4, 2016

MadLittleMods commented Mar 4, 2016

stevenhauser commented Mar 6, 2016

lee-dohm commented Mar 6, 2016

stevenhauser commented Mar 6, 2016

lee-dohm commented Mar 7, 2016

stevenhauser commented Mar 7, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joshaber commented Mar 14, 2016

stevenhauser commented Mar 15, 2016

joshaber commented Mar 15, 2016

		@@ -89,6 +97,9 @@ class PackageGeneratorView extends View

		@runCommand(atom.packages.getApmPath(), args, callback)

		isInPackageMode: ->

atom / package-generator

Support JS package generation #36

Support JS package generation #36

Conversation

stevenhauser commented Nov 22, 2015

stevenhauser commented Nov 22, 2015

stevenhauser commented Nov 22, 2015

This comment has been minimized.

50Wliu Nov 22, 2015 Member

This comment has been minimized.

stevenhauser Nov 22, 2015 Author Contributor

This comment has been minimized.

50Wliu Nov 22, 2015 Member

This comment has been minimized.

stevenhauser Nov 22, 2015 Author Contributor

This comment has been minimized.

50Wliu Nov 22, 2015 Member

stevenhauser commented Nov 22, 2015

50Wliu commented Nov 22, 2015

vinkla commented Dec 2, 2015

stevenhauser commented Mar 4, 2016

MadLittleMods commented Mar 4, 2016

stevenhauser commented Mar 6, 2016

lee-dohm commented Mar 6, 2016

stevenhauser commented Mar 6, 2016

lee-dohm commented Mar 7, 2016

stevenhauser commented Mar 7, 2016

This comment has been minimized.

joshaber Mar 14, 2016 Contributor

This comment has been minimized.

stevenhauser Mar 15, 2016 Author Contributor

This comment has been minimized.

joshaber Mar 15, 2016 Contributor

joshaber commented Mar 14, 2016

stevenhauser commented Mar 15, 2016

joshaber commented Mar 15, 2016

50Wliu Nov 22, 2015
Member

stevenhauser Nov 22, 2015
Author Contributor

50Wliu Nov 22, 2015
Member

stevenhauser Nov 22, 2015
Author Contributor

50Wliu Nov 22, 2015
Member

joshaber Mar 14, 2016
Contributor

stevenhauser Mar 15, 2016
Author Contributor

joshaber Mar 15, 2016
Contributor