Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

rewrite with ES2015 #63

Merged
merged 1 commit into from
Jan 15, 2016
Merged

rewrite with ES2015 #63

merged 1 commit into from
Jan 15, 2016

Conversation

1000ch
Copy link
Member

@1000ch 1000ch commented Jan 7, 2016

This should be reviewed & tested before merging...

ping: @Arcanemagus @johnwebbcole @steelbrain

@Arcanemagus
Copy link
Member

Add a .gitattributes that forces LF EOL as well, or Windows clients will check out the files with CRLF, then the .editorconfig will change them to LF on open... leading to the entire file being "different".

@@ -1,34 +1,18 @@
# linter-htmlhint

This linter plugin for [Linter](https://github.com/AtomLinter/Linter) provides
an interface to [htmlhint](https://github.com/yaniswang/HTMLHint). It will be
A plugin for [Atom Linter](https://github.com/AtomLinter/Linter) providing an interface to [HTMLHint](https://github.com/yaniswang/HTMLHint). It will be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct URL is https://github.com/atom-community/linter, also it's just "Linter".

@Arcanemagus
Copy link
Member

I think that's all for now, it would be nice if we could get specs added as well, but I'm okay with that being a separate PR for now.

@1000ch
Copy link
Member Author

1000ch commented Jan 7, 2016

I think that's all for now, it would be nice if we could get specs added as well, but I'm okay with that being a separate PR for now.

Yes, should be added. I'll send a PR to add specs later.

installed, please follow the instructions [here](https://github.com/AtomLinter/Linter).

### Plugin installation
Linter package must be installed in order to use this plugin. If Linter is not installed, please follow the instructions [Linter].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want an example of what I was talking about, here is one that I recently wrote.

What you have here will work though if you change the last part to:

If [Linter] is not installed, it will be installed for you.

@@ -1,34 +1,23 @@
# linter-htmlhint

This linter plugin for [Linter](https://github.com/AtomLinter/Linter) provides
an interface to [htmlhint](https://github.com/yaniswang/HTMLHint). It will be
A plugin for [Linter](https://github.com/atom-community/linter) providing an interface to [HTMLHint]. It will be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be [Linter] to use your single defined URL down below 😉.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops...!

@Arcanemagus
Copy link
Member

@johnwebbcole Any comments?

@johnwebbcole
Copy link
Contributor

I like the move to JS, I think we should also have a pull request for the Linter API to include a JS plugin example to go with the existing CS example. That way someone who is looking for the Linter way of doing things will also have a JS example.

Has anyone confirmed that APM loads the Linter dependency if it's not already installed? Earlier versions didn't, which was why those instructions were in the readme. If it does, great!

Lets bump the version up to 0.3 too, in case someone wants to live on the CS version.

I haven't had a chance to use bable yet, but have been wanting too. Are there any changes to the build/deploy process?

I'll try the branch out tonight.

@steelbrain
Copy link

@johnwebbcole I've already updated the Linter API wiki page to have a JS provider a long time ago

@Arcanemagus
Copy link
Member

With other plugins we've bumped the major version number when a complete re-write has been done, just in case.

As for babel, inside Atom directly you don't need to do anything special, but if you are spawning an external thread (such as the worker process in linter-eslint) it needs to be ES5 only apparently.

@steelbrain could tell you more as I've mainly worked directly in Atom so far.

@steelbrain
Copy link

@Arcanemagus Yes that's true, Atom intercepts each require and tries to compile it if it's a babel file but if it's on a spawned or even forked node process, it won't setup those compilation hooks. and to overcome the storage limitations of APM, you'll have to keep compiled babel files in the repo

@Arcanemagus
Copy link
Member

@johnwebbcole the "package-deps" section, plus the require('atom-package-deps).install() in the activate section are what automatically install the Linter package.

I've had AtomLinter/Meta#8 open for a while regarding a "template" package, but haven't had a chance to actually implement it yet.

Anyone have a reason this shouldn't be merged? LGTM currently, assuming it works 😛.

@@ -0,0 +1,80 @@
'use babel';

import path from 'path';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a side note that this will break in the future versions of node because they don't export a default for you to use here, either do

import * as path from 'path'

or do

import {dirname as getDirNameFromPath} from 'path'

^ add anything you wanna import there

@steelbrain
Copy link

Overall, @1000ch I like what you did here. Good job 👍

@1000ch
Copy link
Member Author

1000ch commented Jan 15, 2016

@steelbrain Also fixed provideLinter() & squashed commits 👍

@steelbrain
Copy link

LGTM

@Arcanemagus
Copy link
Member

Merging it in since nobody else seems to be doing so 😛.

Wwe can fix any remaining issues from there.

Arcanemagus added a commit that referenced this pull request Jan 15, 2016
@Arcanemagus Arcanemagus merged commit caf8440 into master Jan 15, 2016
@Arcanemagus Arcanemagus deleted the rewrite branch January 15, 2016 21:02
@1000ch
Copy link
Member Author

1000ch commented Jan 16, 2016

I just published v0.3.0. I will publish v1.0.0 after adding specs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants