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

Do not try to write back ignored files after --fix. #2652

Merged
merged 2 commits into from Jun 26, 2017

Conversation

4 participants
@subzey
Contributor

subzey commented Jun 19, 2017

Which issue, if any, is this issue related to?

Fixes issue #2627

Is there anything in the PR that needs further explanation?

Accessing opts.syntax throws if the file is ignored. I wonder why Flow didn't catch that?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 19, 2017

Thank you for a fix! Would you mind adding test for this issue, please? You can add it to fix.test.js.

@subzey

This comment has been minimized.

Contributor

subzey commented Jun 19, 2017

@hudochenkov Looks like fix.test.js contains tests for lib version of stylelint (lib/index), and I need to test the standalone script (lib/standalone). I'm not really sure how to do that, in fact I can see no tests that contains "standalone" to start off from.

I definitely need help with tests.

@subzey

This comment has been minimized.

Contributor

subzey commented Jun 20, 2017

Added a test. Not sure I did it right, but at least linters are okay, and it's green with the proposed fix code and red with the old one

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 21, 2017

Thank you for thinking outside the box for test :)

I came up with this test:

it("doesn't write to ignored file", () => {
  return stylelint.lint({
    files: [stylesheetPath],
    config: {
      ignoreFiles: stylesheetPath,
      rules: {
        "at-rule-name-case": "lower",
        "comment-empty-line-before": "always",
        "comment-no-empty": true,
      },
    },
    fix: true,
  }).then(() => {
    return pify(fs.readFile)(stylesheetPath, "utf8").then((newFile) => {
      return pify(fs.readFile)(path.join(__dirname, "stylesheet.css"), "utf8").then((oldFile) => {
        expect(newFile).toBe(oldFile)
      })
    })
  })
})

I still don't understand promises, so this why it's ugly as hell. If someone could improve code it would be much appreciated :)

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 21, 2017

@hudochenkov promises is very ugly if your don't use async/await 😄

@subzey

This comment has been minimized.

Contributor

subzey commented Jun 22, 2017

@hudochenkov hudochenkov referenced this pull request Jun 26, 2017

Closed

Release 7.12 #2574

5 of 5 tasks complete
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 26, 2017

@hudochenkov Is this good to go in for 7.12 or did you want to change the test? Is there any advantage to either approach over the other?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 26, 2017

@jeddy3 it's good to merge, if test looks ok.

Is there any advantage to either approach over the other?

Test in this PR looks quite complicated. While my version might be improved. I didn't ask to use my test, because not sure if they are better than @subzey's test.

If you accept author's test, then feel free to merge.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 26, 2017

Let's merge and then I'll drop in your test, if only because it's more consistent with the other tests in the file.

@jeddy3

jeddy3 approved these changes Jun 26, 2017

@jeddy3 jeddy3 merged commit d4a8107 into stylelint:master Jun 26, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 95.631%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 26, 2017

Changelog:

  • Fixed: --fix no longer crashes when used with ignored files (#2652).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment