cfpb / generator-cf

Yeoman generator for Capital Framework
http://cfpb.github.io/capital-framework/getting-started/
Creative Commons Zero v1.0 Universal
9 stars 13 forks source link

Fetch .eslintrc from front-end repo #64

Closed Scotchester closed 9 years ago

Scotchester commented 9 years ago

This PR:

  1. Creates an osLibraries array at the top of index.js
  2. Sets up the downloadTemplate function to consume that array and fetch the contents of each of its included URLS before moving on to the appFiles function.

    This enables us to add more source repos in the future, if needed.

  3. Copies the .eslintrc file from the front-end repo cache to the new project.
  4. Removes the old manually-updated .eslintrc template file.

Review:

Closes #58

ascott1 commented 9 years ago

:+1:

Scotchester commented 9 years ago

Added a commit to fix two problems on the initial run of grunt:eslint after generation:

First, our latest ESLint file uses a rule that was added in ESLint 0.16.0.

Running "eslint:target" (eslint) task
Warning: Definition for rule 'semi-spacing' was not found. Use --force to continue.

To fix this, I updated the grunt-eslint version number to 11.0.0.

Also, the template's app.js file threw two line-length warnings, so I edited it to fix that.

Scotchester commented 9 years ago

On the subject of dependency pinning:

Why are we inconsistent about what gets ~ and what gets ^? Seems like we should pick a position and stick to it.

Personally, I like ~ because I feel like, for the most part, we can trust package authors to not break compatibility with patch-level releases. But @contolini made an argument for hard pinning the other day.

Should I make this an issue in cfpb/front-end?

anselmbradford commented 9 years ago

our latest ESLint file uses a rule that was added (renamed?) in ESLint 0.19.0.

semi-spacing was added in 0.18.0., there are additional rules added in 0.19.0. that we'll want to include in https://github.com/cfpb/front-end/blob/master/.eslintrc

Scotchester commented 9 years ago

Actually, it was 0.16.0. I misunderstood the Git tag display at the top of this commit: https://github.com/eslint/eslint/commit/151bc5021475af44b053e1a38e908c34858f9060

But yeah, you're right about updating with the new rules.

contolini commented 9 years ago

I don't think I made an argument for hard-pinning. I think I said if you feel you need hard-pinning, use npm shrinkwrap to create a shrinkwrap file.

The mix of tildes and carets is because npm started defaulting to carets last year.

jimmynotjim commented 9 years ago

Nice work @Scotchester :+1:

Scotchester commented 9 years ago

@contolini Fair enough, but do we agree that we should pick one or the other and standardize on it?

contolini commented 9 years ago

@Scotchester It doesn't really bug me but it looks like you can change the default if the team wants to standardize on something. I lean toward the caret because a) it's what the larger community uses and b) there's nothing wrong with getting new API-compatible functionality provided by minor releases (assuming, of course, the author has used semver correctly).

contolini commented 9 years ago

Oh and before you squash this, have the test check for .eslintrc by adding it to the end of this array.

Scotchester commented 9 years ago

I did, upon fuller reading of that article note that the caret works in the same way as the tilde for 0.x.x releases, which makes me feel better about it.

I'm in favor of standardizing on the caret. Other opinions?

Scotchester commented 9 years ago

Re: checking for .eslintrc. How do I run the tests?

contolini commented 9 years ago

npm test from the project root. I'll add some testing notes to the readme.

Scotchester commented 9 years ago

It's testing the state of my local repo, right? The first line of the output is

> generator-cf@0.5.0 test /Users/cranfills/Projects/Capital Framework/generator-cf
contolini commented 9 years ago

Yeah, that looks correct. You can double check the test is working correctly by adding a fake filename to that array and the test should fail.

Scotchester commented 9 years ago

:+1: Tests pass.