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

Ensure absolute paths to processors #2207

Merged
merged 1 commit into from Dec 29, 2016

Conversation

3 participants
@davidtheclark
Contributor

davidtheclark commented Dec 24, 2016

Closes #2194, closes #2195.

Although this does not add more tests, it does add some errors that break things (causing tests to fail) if absolute paths are for some reason not coming through. More focused unit tests looked like more trouble to figure out than they were worth, so I think this does the trick.

I'd like to cut a release for this as soon as possible, so processors stop failing. How about finishing up this and #2197 and then releasing?

@davidtheclark davidtheclark force-pushed the processor-paths branch from f6d43a4 to 1b985ca Dec 24, 2016

@m-allanson

This comment has been minimized.

Member

m-allanson commented Dec 24, 2016

@davidtheclark it might be nice to include #1663 in the next release too, so the deprecations are landed at once. Although I've not done any work on that yet.

@m-allanson

One question, but otherwise LGTM.

return (root, result) => {
let filePath = options.from || _.get(root, "source.input.file")
if (filePath !== undefined && !path.isAbsolute(filePath)) {

This comment has been minimized.

@m-allanson

m-allanson Dec 24, 2016

Member

Is this pattern the same here and in standalone.js? If so, is it possible to create a util named something like absolutizePath() and use that?

This comment has been minimized.

@davidtheclark

davidtheclark Dec 26, 2016

Contributor

I don't think this pattern is util-worthy. It's just very straightforward JS.

@@ -51,7 +52,13 @@ module.exports = function (options/*: Object */)/*: Promise<stylelint$standalone
})
if (!files) {
return stylelint._lintSource({ code, codeFilename }).then(postcssResult => {
const absoluteCodeFilename = (codeFilename !== undefined && !path.isAbsolute(codeFilename))

This comment has been minimized.

@m-allanson

m-allanson Dec 24, 2016

Member

See comment about creating a util.

@@ -70,7 +77,12 @@ module.exports = function (options/*: Object */)/*: Promise<stylelint$standalone
}
const getStylelintResults = filePaths.map(filePath => {
return stylelint._lintSource({ filePath }).then(postcssResult => {
const absoluteFilepath = (!path.isAbsolute(filePath))

This comment has been minimized.

@m-allanson

m-allanson Dec 24, 2016

Member

See comment about creating a util.

@jeddy3 jeddy3 merged commit e00b3eb into master Dec 29, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jeddy3 jeddy3 deleted the processor-paths branch Dec 29, 2016

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Dec 29, 2016

Added to changelog as:

  • Fixed: ensure only absolute filepaths are passed to processors (#2207).

I'd like to cut a release for this as soon as possible, so processors stop failing.

Oops, I just saw this. I got caught up eating mince pies. I can probably do a release today - I'll create an issue.

it might be nice to include #1663 in the next release too, so the deprecations are landed at once.

The final list of deprecations still needs to be decided on. Hence the deprecations branch. I hope to have time to contribute to the various rule discussions forward in the next few days. In the meantime I don't think there's anything blocking a release with this processors fix.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Dec 29, 2016

Hope you enjoyed your holiday!

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Dec 30, 2016

I did thank you! I trust you had an enjoyable xmas too! :)

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

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