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

Bring back line ending correction #258

Merged
merged 3 commits into from
Mar 24, 2017
Merged

Bring back line ending correction #258

merged 3 commits into from
Mar 24, 2017

Conversation

mischah
Copy link
Member

@mischah mischah commented Mar 24, 2017

Relates to #257, #251

Must say that I’m not sure if I’m super happy with that because using the line ending corrector was a no brainer before. It was bundled into the gulp default task as well as the watch task.

So I’m asking what to do about it.
Ideas:

  • Document that with more details in the README?
    • I guess people won’t read this 😕
  • Add an additional npm script like build ?
    • So the scripts section could look like:
      "scripts": {
          "prepublish": "nsp check",
          "pretest": "eslint **/*.js --fix",
          "test": "jest",
          "fixLineEndings": "lec lib/cli.js"
          "build": "npm run fixLineEndings && npm test"
      }
      

Any feedback appreciated 😗

@zckrs
Copy link
Member

zckrs commented Mar 24, 2017

Initialy we fix line endings in a prepublish gulp task.

So

"scripts": {
    "prepublish": "lec lib/cli.js && nsp check",
    "pretest": "eslint **/*.js --fix",
    "test": "jest"
}

@mischah
Copy link
Member Author

mischah commented Mar 24, 2017

Jup, also thought about that.
But Isn’t it a bit weird to run a CLI App which potentially changes files in the »prepublish« phase?

Reason: This might change a file which make it in the changed state into your npm release but the changes aren’t in your version control at this time 😳

Get what I’m trying to say?

@zckrs
Copy link
Member

zckrs commented Mar 24, 2017

This might change a file which make it in the changed state into your npm release but the changes aren’t in your version control at this time

Agree 🤔

@SBoudrias
Copy link
Member

But that's how it was before. This really is a hot fix for a common mistake that'll break a package on OSx

… and remove info about it from readme.
@mischah
Copy link
Member Author

mischah commented Mar 24, 2017

Alright. Pushed another commit to change that.

@SBoudrias SBoudrias merged commit 5c510a8 into master Mar 24, 2017
@SBoudrias
Copy link
Member

great :)

@SBoudrias SBoudrias deleted the line-ending-corrector branch March 24, 2017 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants