Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint a string #161

Closed
wants to merge 2 commits into from
Closed

Lint a string #161

wants to merge 2 commits into from

Conversation

SimenB
Copy link
Owner

@SimenB SimenB commented Jul 24, 2015

⚠️ Not ready for merge

A small start to being able to lint strings. Fixes #146 & #160

How to call it is a bit undecided, but for now it's

var stylint = require('stylint')

stylint.api(optionalConfig).lintText(stringWithStylus, optionalConfig, optionalFilename)

Returned from that is an array containing all violations.
Sample using default options:

import {api} from 'stylint'
const stylintInstance = api()
// ... Some time later
stylintInstance.lintText('.class {\n  color: red !important\n}\n', null, 'filename.styl')

returns

[ { message: 'unecessary bracket',
    severity: 'Warning',
    file: 'filename.styl',
    lineNo: 1,
    origLine: '.class {' },
  { message: '!important is disallowed ',
    severity: 'Warning',
    file: 'filename.styl',
    lineNo: 2,
    origLine: '  color: red !important' },
  { message: 'missing semicolon',
    severity: 'Warning',
    file: 'filename.styl',
    lineNo: 2,
    origLine: '  color: red !important' },
  { message: 'unecessary bracket',
    severity: 'Warning',
    file: 'filename.styl',
    lineNo: 3,
    origLine: '}' } ]

Currently working on restructuring how reporters are called. Moving the logic for it to done, so that done calls reporters and not the other way around

@SimenB
Copy link
Owner Author

SimenB commented Jul 26, 2015

@rossPatton Ok, could you take a quick look? I'd still like to completely decouple reporter from this, to allow easier use from outside of the CLI, like from grunt/gulp plugin (avoid state leaking). So if you have a big array of results, you can just pass them through a reporter without it really having to be inside stylint at all. Make sense?

Anyways, what do you think of the general direction?

These are breaking changes btw. It would be possible without it, but that'd require even more magic state everywhere, which I just find confusing 😆 I think idempotency should be a goal when using the programmatic approach, or at least make state opt-in like eslint, to avoid confusing behavior.

@@ -48,6 +48,7 @@
"chokidar": "1.0.1",
"glob": "4.3.1",
"lodash.defaults": "3.1.2",
"lodash.groupby": "^3.1.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove the ^ here? I try to keep deps locked down as much as possible

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, sure. This is just me doing npm i -S

@rossPatton
Copy link
Collaborator

I like this direction and approach, but I'm worried about introducing breaking changes so soon after 1.0.0, and when there are still some bugs I haven't fixed yet.

I'm not opposed to merging it in, just that I would wait a bit and take care of some other things first.

If there's a way to do it without breaking changes, I'd be 100% down to merge it in right away, and then maybe we transition to this version later?

@SimenB
Copy link
Owner Author

SimenB commented Jul 27, 2015

This is just breaking for programmatic usage, CLI is the same.
As noted, it is possible to do this non-breaking, but that would entail a couple of boolean parameters. Not a real problem of course, but the API wouldn't be as clean as it could (should?) be.

@SimenB
Copy link
Owner Author

SimenB commented Jul 27, 2015

The breaking part btw is that calling this.reporter(null, 'done') doesn't call this.done, but done calls the reporter.
So using the API with create and all should work the same but calling what I' d call internal API, (reporter, done etc.) is breaking. When importing stylint also exports every method, it's hard making non-breaking changes without introducing even more state. But yeah, I have no real issue saving those changes for a 2.0 release 😄

Also, make done call reporter, not the other way around
@SimenB
Copy link
Owner Author

SimenB commented Apr 10, 2016

Unable to change target branch, so closing. See #274

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

Successfully merging this pull request may close these issues.

2 participants