search

AtomLinter / linter-jshint

Atom linter plugin for JavaScript, using jshint.
147 stars 39 forks source link

Lint grammar files "JavaScript for Automation (JXA)" #310

Closed dagware closed 7 years ago

dagware commented 8 years ago

Is there any way to have linter-jshint process files that have the Grammar specified as "JavaScript for Automation (JXA)"?

The only way I can use it right now is to change the Grammar to "JavaScript", then change it back, and that's no fun. :)

Arcanemagus commented 8 years ago

Are all "JavaScript for Automation (JXA)" files perfectly valid JS?

dagware commented 8 years ago

Yes. They're referred to as "JavaScript for Automation" (aka JXA) because Apple makes certain OSA (Open Script Architecture) libraries available to them, and executes them in an environment that allows for access of OS functions, etc.

They can be identified by the fact that they have the language "language-javascript-jxa" (https://atom.io/packages/language-javascript-jxa https://atom.io/packages/language-javascript-jxa) specified.

On Jul 25, 2016, at 12:16 PM, Landon Abney notifications@github.com wrote:

Are all "JavaScript for Automation (JXA)" files perfectly valid JS?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/AtomLinter/linter-jshint/issues/310#issuecomment-235055011, or mute the thread https://github.com/notifications/unsubscribe-auth/ASk6umO6XNulB9aiALwqEEhqQw69_7OGks5qZQuEgaJpZM4JTZM-.

Arcanemagus commented 8 years ago

Cool, then it should be as simple as adding source.js.jxa to the list of allowed scopes here, if you want to submit a PR adding that it would be great.

You'll want to add a spec testing that it works for those files, see the CI files in linter-stylelint for how to pre-install the language file, and remember to activate the language in the relevant section of the spec file.

dagware commented 8 years ago

OK, that's cool, except I've never done any of that before. Not sure what a "PR" is, although I might assume the "R" stands for request. Not sure how to do the rest of the stuff, either.

With that said, I'm a developer, so I can learn just about anything. Unfortunately, I've spent my (long) professional career on the Windows side, so even though I'm all about the Mac now, I'm fighting an uphill battle with all the things I don't know.

So if you want to point me to some docs to learn how to do all this, I'd be happy to learn. Just be aware that I may be a pest with stupid questions until I get it figured out. It's OK with me if it's OK with you, but you've been warned. :)

On Jul 26, 2016, at 9:55 AM, Landon Abney notifications@github.com wrote:

Cool, then it should be as simple as adding source.js.jxa to the list of allowed scopes here https://github.com/AtomLinter/linter-jshint/blob/5809510d7a8674baa7b3b2fc53e76a2dabb66c40/lib/main.js#L41, if you want to submit a PR adding that it would be great.

You'll want to add a spec testing that it works for those files, see the CI files in linter-stylelint https://github.com/AtomLinter/linter-stylelint for how to pre-install the language file, and remember to activate the language https://github.com/AtomLinter/linter-stylelint/blob/1dc983a371ce839ae99d8c41052ddfde0f03b596/spec/linter-stylelint-spec.js#L213-L216 in the relevant section of the spec file.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/AtomLinter/linter-jshint/issues/310#issuecomment-235332511, or mute the thread https://github.com/notifications/unsubscribe-auth/ASk6us1svJPtPhZkDU3oYM9cYhe2UUdpks5qZjwCgaJpZM4JTZM-.

Arcanemagus commented 8 years ago

"PR" = "Pull Request", or how you say "I have these changes that I would like to make, will you accept them?" to a project.

I can take care of this at some point if you don't want to go through the trouble, but on the other hand if you want a learning experience I'd be happy to help you through the process, it's up to you 😉.

dagware commented 8 years ago

I'd love to learn the process. I've just started working with GitHub, and I'm getting a crash course in Atom (I love it), so this is the next logical step.

What steps do I follow for a PR? I clicked on the button for creating a new pull request, but I wasn't sure what it was asking, so I stopped.

On Jul 26, 2016, at 10:48 AM, Landon Abney notifications@github.com wrote:

"PR" = "Pull Request", or how you say "I have these changes that I would like to make, will you accept them?" to a project.

I can take care of this at some point if you don't want to go through the trouble, but on the other hand if you want a learning experience I'd be happy to help you through the process, it's up to you 😉.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/AtomLinter/linter-jshint/issues/310#issuecomment-235348539, or mute the thread https://github.com/notifications/unsubscribe-auth/ASk6unZBQnoVGX64E89NrHfwUJH4cPQZks5qZkhYgaJpZM4JTZM-.

Arcanemagus commented 8 years ago

First you will want to fork this repository to your account (button at the top right of the page). Then create a new branch for the work you will be doing. You can do this in the web UI, but as you will need something to manage the code on your computer anyway, you should probably just do it there. If you are just starting I would recommend installing GitHub Desktop as it works well enough for beginner users.

When creating your branch give it a descriptive, but short, name. Something like "add-jxa-support" in this case. Your client should automatically switch you to that branch when you create it.

Next you will need to make the required changes. Like I said before you need to add the "scope" (how Atom determines what a file actually is) for JXA files to the list of scopes this linter plugin claims it supports. You would want to add that right here.

Next create a "jxa" folder in the spec/fixtures section, and put a file that has no problems, and one that has at least one problem with it in that folder.

Now that you have the support added there, and the files you will be testing added, you need to tell it how to test that everything is working. Start this by storing the path to those files at the top of the spec/linter-jshint-spec.js file, like you see for the 4 file paths already there. Next I would copy an existing section of the spec and just modify it to suit your needs. Change the describe to something like "works with JXA files", modify the beforeEach to have a line like this:

waitsForPromise(() => atom.packages.activatePackage('language-javascript-jxa'));

To tell Atom it should load that language file for this section of the specs. Then modify the "verifies the first message" block to match the message from your "bad" file, changing the expected message, and the range.

You can then run the spec by running View -> Developer -> Run Package Spec from within Atom while editing the code, or by running apm test from the package folder. Fix any mistakes in the code and run again till the specs all pass.

Once you have it passing locally you will need to ensure the CI environment is setup to properly test it is working in all environments. Start by ensuring the extra language file is installed:

.travis.yml: Change this section to look like:

env:
  global:
    - APM_TEST_PACKAGES="language-javascript-jxa"
  matrix:
    - ATOM_CHANNEL=stable
    - ATOM_CHANNEL=beta

appveyor.yml: Change this section to look like:

environment:
  APM_TEST_PACKAGES: language-javascript-jxa

  matrix:
  - ATOM_CHANNEL: stable
  - ATOM_CHANNEL: beta

circle.yml: Right after this line, add the following:

apm install language-javascript-jxa

At this point you should be done with the required changes, so commit everything with a descriptive commit message, and push your changes up to your branch. I think the GitHub Desktop client actually lets you start a PR directly from there, otherwise go back to the web interface and start a PR with your branch you created as the "source" and the master branch of this repo as the destination. It should automatically fill in the PR text with your commit message. Once you submit the PR, it will show up here for review.

dagware commented 8 years ago

Thanks. I'll get to this this afternoon (i.e. in a few hours). I'm reasonably familiar with Git and GutHub for basic version control stuff, so that part should be easy. I'll let you know how the rest goes.

Thanks!

dagware commented 8 years ago

Well, you've just made a huge mistake. :) I've been looking for a testing suite, and now you've inadvertently led me to Jasmine. I can't resist the rabbit trail, as I desperately needed something like this.

So it might be a day or two before I'm back from this trail. I will get back, rest assured, but not today. :)

dagware commented 8 years ago

I tried to run the test, but I think I don't have everything installed I need. I got this error:

Error: Cannot find module '/Users/Dan/Documents/Development/Atom/spec'
    at Module._resolveFilename (module.js:338)
    at Function.Module._resolveFilename (/Applications/Atom.app/Contents/Resources/app.asar/src/module-cache.js:383)
    at Function.Module._load (module.js:289)
    at Module.require (module.js:366)
    at require (/Applications/Atom.app/Contents/Resources/app.asar/src/native-compile-cache.js:50)
    at requireSpecs (/Applications/Atom.app/Contents/Resources/app.asar/spec/jasmine-test-runner.js:99)
    at module.exports (/Applications/Atom.app/Contents/Resources/app.asar/spec/jasmine-test-runner.js:47)
    at module.exports (/Applications/Atom.app/Contents/Resources/app.asar/src/initialize-test-window.js:72)
    at setupWindow (index.js:87)
    at window.onload (index.js:41)handleSetupError @ index.js:62
Arcanemagus commented 8 years ago

You should be in the directory containing the code for this project before running apm test.

Btw, as far as test suits are concerned Jasmine (v1.3) is just what is bundled into Atom, I've heard a lot of people prefer Mocha so you might want to look into that as well for your own projects 😉.

dagware commented 8 years ago

Sorry - I posted the wrong error message. This is the one I meant to post:

Last login: Wed Jul 27 15:56:35 on ttys000
Dans-iMac:linter-jshint Dan$ apm test
[52912:0727/155649:WARNING:resource_bundle.cc(305)] locale_file_path.empty() for locale English
[52916:0727/155650:WARNING:resource_bundle.cc(305)] locale_file_path.empty() for locale English
[52912:0727/155650:INFO:CONSOLE(52)] "Window load time: 468ms", source: file:///Applications/Atom.app/Contents/Resources/app.asar/static/index.js (52)
F

The JSHint provider for Linter
  it encountered a declaration exception
    Error: Cannot find module 'atom-linter'
      at Module._resolveFilename (module.js:338:15)
      at Function.Module._resolveFilename (/Applications/Atom.app/Contents/Resources/app.asar/src/module-cache.js:383:52)
      at Function.Module._load (module.js:289:25)
      at Module.require (module.js:366:17)
      at require (/Applications/Atom.app/Contents/Resources/app.asar/src/native-compile-cache.js:50:27)
      at Object.provideLinter (/Users/Dan/Documents/Development/Atom/linter-jshint/lib/main.js:90:21)
      at [object Object].<anonymous> (/Users/Dan/Documents/Development/Atom/linter-jshint/spec/linter-jshint-spec.js:14:23)
      at Object.<anonymous> (/Users/Dan/Documents/Development/Atom/linter-jshint/spec/linter-jshint-spec.js:13:1)
      at Module._compile (/Applications/Atom.app/Contents/Resources/app.asar/src/native-compile-cache.js:103:30)
      at Object.defineProperty.value [as .js] (/Applications/Atom.app/Contents/Resources/app.asar/src/compile-cache.js:208:21)
      at Module.load (/Applications/Atom.app/Contents/Resources/app.asar/node_modules/coffee-script/lib/coffee-script/register.js:45:36)
      at Function.Module._load (module.js:313:12)
      at Module.require (module.js:366:17)
      at require (/Applications/Atom.app/Contents/Resources/app.asar/src/native-compile-cache.js:50:27)
      at module.exports (/Applications/Atom.app/Contents/Resources/app.asar/src/initialize-test-window.js:72:17)
      at setupWindow (file:///Applications/Atom.app/Contents/Resources/app.asar/static/index.js:87:12)
      at window.onload (file:///Applications/Atom.app/Contents/Resources/app.asar/static/index.js:41:9)

Finished in 0.093 seconds
1 test, 1 assertion, 1 failure, 0 skipped

Tests failed
Dans-iMac:linter-jshint Dan$

Thanks for the comment about Mocha. I'll take a look.

Arcanemagus commented 8 years ago

Looks like you need to run npm install in the package directory (apm install will also work for now, but it uses an ancient version of Node.js, so you should use Node's npm if available).

dagware commented 8 years ago

OK, that worked, but now I don't really know how to code the tests. Specifically, when using the waitsForPromise looking for an error message, I don't know how to figure out what exactly it should be looking for.

Really, the existing tests should work just the same for a jxa file as for a js file. I could just duplicate the entire set of tests, just changing the language, but there's probably a better way than that.

I'm beginning to think this is over my head, for the time being. I mean seriously, like you said, all that's needed is a minor addition to one line.

augustobmoura commented 7 years ago

374 solves it. The source.js.jxa scope can be added in the list to be linted.

Arcanemagus commented 7 years ago

Marking this as a duplicate of #372, since that moves the responsibility of making sure it works out to the user, bypassing all the extra steps required to ensure it is working that was being outlined above.