@nickjs, just to check; is the linting also being run on the files in ./assets/_scss? Did those files not report any errors?

@msecret No, sorry, to be clear there are still a tonnnn of linter warnings all across the repo. I just concentrated on styleguide.scss for now. Subsequent PRs could fix other files. I also didn't add a postcommit hook or anything, so it's not like every time you try to commit you're going to get hundreds of warnings in your face. You still need to run scss-lint manually.

Would it be possible to have scss-lint only run on files within the _scss directory? The main.scss file for the assets directory only import files from the _scss directory and doesn't need to be linted. This would solve the issue around yaml front-matter.

Also, @nickjs, there are two big issues landing in the next release. These issues are #975 and #974. The latter issue will refactor a lot of the project and separate code from the site and the actual library. The former issue is one I'll be working on, and I'd like to have these changes integrated into that PR. For now we can leave this PR open to discuss this more, but eventually we can work in tandem to get these changes in once those two issues are closer to being resolved.

@rogeruiz Just pushed a change to the scss-lint config to only run on _scss and exclude _scss/lib. Ran it through wc, a default scss-lint with no arguments generates 234 warnings and 0 errors. Honestly, it seems like a lot of them can be easily grepped, I'd be willing to do that if we decide it's worth it to add the linting.

Because of the Jekyll front matter thing I mentioned above, you still need to manually lint styleguide.scss via scss-lint assets/css/styleguide.scss. If we can remove that front matter, we can add it to the default list of files, which will help until the refactor happens in #974.

So, has any work started on either of those issues? Super excited especially for #974, but if not, I'd recommend merging this now. It adds scss-lint without any side effects since it's not integrated into the automated release process. It will be a lot easier for you later on to just add the existing scss-lint to the build mechanism than it would be to add a whole new tool. Plus, if code is eventually going to get refactored, this PR will eventually just be one big merge conflict, so better to merge it in now, no? I'm not gonna be mad if styeguide.scss (and my little refactor here) disappear later on :)

I'd be fine adding this now, it's a good start. We could also set the config to lint _scss and setup HoundCI because it will only complain about new code, which would be a great way to move toward reducing the linting errors.

I can commit to cleaning up at least 100 warnings if we merge this ;)

Good idea, let's merge this in. Should we include any documentation on this
so far? Or are we going to be using HoundCI for now since it's a manual
process?

On Mon, Feb 1, 2016 at 3:52 PM, Marco Segreto notifications@github.com
wrote:

I'd be fine adding this now, it's a good start. We could also set the
config to lint _scss and setup HoundCI because it will only complain
about new code, which would be a great way to move toward reducing the
linting errors.

—
Reply to this email directly or view it on GitHub
#976 (comment)
.

Roger Steve Ruiz
804-539-2255
Innovation Specialist @ 18F
General Services Administration
1800 F St. NW, Washington, DC 20405

@msecret already added the config option to lint _scss by default when you run scss-lint (excluding lib/) :)

@rogeruiz just updated the README with linting instructions. Good call.

Totally up to you guys if you want to enable Hound, though still probably good to do a pass at removing as many warnings as possible first.

@maya thanks, fixed.

This is awesome work! I'd like us to include this too. Can you clarify what file(s) we have to remove frontmatter from in order to run scss-lint?

As a side note, we didn't spend a lot of time refactoring the styleguide.scss bc it wasn't part of our distributed code, but thanks for the improvements!

@maya actually I think the only files I can find that have it that we would care about would be assets/css/styleguide.scss and assets-styleguide/css/homepage.scss. I'm not sure what the difference between those two directories is and if they both should be linted, nor am I familiar enough with Jekyll to say whether the front matter can be removed or not.

I figured, and I figured the code could change at any point, so I didn't spend too much time on it aside from getting it to (mostly) lint (the exception being the SelectorDepth and NestingDepth warnings).

assets-styleguide/css/homepage.scss are the styles for the homepage https://playbook.cio.gov/designstandards/ and assets/css/styleguide.scss are the styles for the styleguide site https://playbook.cio.gov/designstandards/getting-started/ Both import the web design standards SCSS.

So I think we should commit this change as is, ensuring that assets/_scss is linted. From here we'll add HoundCI at some point to move out codebase closer to consistency.

@nickjs, if you'd like to take a crack at fixing a bunch of the errors, we'd be very thankful. Maybe that can go in another PR

Hey @nickjs. This looks great! Thanks for taking the time to add scss-lint to the project and using our own 18F standards as a config. For now, the front matter issue is mentioned in the README. After #974 and #975, the work done here will be integrated and the front matter issue should be taken care of.

Thanks again! 🚀 🎸

🎉 Thanks @rogeruiz!

@msecret Just to be clear, assets/_scss is currently linted when you just run a default scss-lint. As for what mechanism you choose to run that (Hound, cmd, etc), that is an exercise I leave to the reader :)

@nickjs now that this is merged, we're gonna hold you to those 💯 warnings. Just kidding 😉

Thanks again for the awesome work! ⚡

@maya #987 💥