Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Adjust linters. #370

Merged
merged 15 commits into from
Apr 1, 2019
Merged

Adjust linters. #370

merged 15 commits into from
Apr 1, 2019

Conversation

sherakama
Copy link
Member

@sherakama sherakama commented Mar 28, 2019 •
edited
Loading

READY

Summary

  • Our codeclimate score suffered when we moved the code around as all the old issues came back.
  • Looking at the documentation it appears that codeclimate was not respecting our lint configuration file. We had scss-lint enabled in codeclimate but we had configuration for sass-lint in our repo. They are two different things and need to be one.
  • I revised the sass-lint.yml to cut down on errors and have made the first pass at clean up. I think I have gotten it down to real issues we should fix now.
  • The linter picked up a few bugs along the way. Should be fixed now.
  • Also, turn off markdown linting cuz no1curr.

Needed By (Date)

  • Meh

Urgency

  • Low

Steps to Test

  1. Review documentation
  2. Review changes to the sass-lint.yml file
  3. Review code changes
  4. Run a npm run build
  5. Run a npm run sasslint
  6. Leave comments and make changes.

See Also

@sherakama
Copy link
Member Author

Sorry, this is not ready. Needs tweaks.

@sherakama
Copy link
Member Author

@josephgknox @JBCSU @yvonnetangsu

So I finally figured out why our sass lint settings were different in our code climate results. We were using the wrong linter 🤦‍♂️. This PR revises the linting configuration and takes a first comb through the issues to boil them down to 'real' ones.

yvonnetangsu
yvonnetangsu requested changes Mar 29, 2019
View reviewed changes
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add one rule to allow attribute selectors? Useful for [aria-expanded="true"] type of attributes in main nav for example.

yvonnetangsu and others added 2 commits March 29, 2019 09:44
@yvonnetangsu @sherakama
Update .sass-lint.yml
529ecd1 
Co-Authored-By: sherakama <sherakama@gmail.com>
@sherakama
fixup. wip.
bf37445
@JBCSU
Copy link
Collaborator

JBCSU commented Mar 30, 2019

@sherakama I'm not sure I understand how to test this PR. I followed the instructions

  • npm run build
  • npm run sasslint

and I got 1131 warnings, including tons of warnings about whitespace and semicolons. When I look at the codeclimate details there are 7 issues that aren't marked Fixed, none of which are about whitespace or semicolons. So it seems to me that the lint configurations don't match. What am I looking for in order to validate this PR?

@JBCSU
Copy link
Collaborator

JBCSU commented Mar 30, 2019

FYI, CC's issue with not allowing filename extensions on @import statements is referring to the @import bourbon.scss directives in our 3 rollup .scss files. The .scss extension is required to work around a bug in webpack's sass-loader: bourbon has both a bourbon.js and bourbon.scss rollup. When sass-loader is processing a .scss file with @import bourbon, and both a .js and a .scss file could resolve, it incorrectly tries to import the .js file. We should keep an eye on the sass-loader package. When that bug is fixed we can remove the .scss extension from our @imports.

@yvonnetangsu
Copy link
Member

@sherakama I'm not sure I understand how to test this PR. I followed the instructions

  • npm run build
  • npm run sasslint

and I got 1131 warnings, including tons of warnings about whitespace and semicolons. When I look at the codeclimate details there are 7 issues that aren't marked Fixed, none of which are about whitespace or semicolons. So it seems to me that the lint configurations don't match. What am I looking for in order to validate this PR?

Weird....when I ran it last time, I got 0 error and 2x warnings 🤔

@josephgknox
Copy link

@sherakama I'm not sure I understand how to test this PR. I followed the instructions

  • npm run build
  • npm run sasslint

and I got 1131 warnings, including tons of warnings about whitespace and semicolons. When I look at the codeclimate details there are 7 issues that aren't marked Fixed, none of which are about whitespace or semicolons. So it seems to me that the lint configurations don't match. What am I looking for in order to validate this PR?

Weird....when I ran it last time, I got 0 error and 2x warnings 🤔

+1. 🤔

@JBCSU
Copy link
Collaborator

JBCSU commented Mar 30, 2019 •
edited
Loading

Y'know what I should probably do? I should probably test your branch, instead of master. That might help.

D'oh! 🤦‍♀️ Sorry 'bout that. My brain's fried after 2 days of training.

@JBCSU
Copy link
Collaborator

JBCSU commented Mar 30, 2019 •
edited
Loading

When I test the correct branch, 0 errors, 28 warnings. That's much mo' bettah. Thanks for doing this. It makes linting actually useful.

@sherakama sherakama merged commit 984cbca into master Apr 1, 2019
@sherakama sherakama deleted the lnt branch April 1, 2019 16:03
yvonnetangsu added a commit that referenced this pull request Jun 11, 2019
@yvonnetangsu
Merge branch 'master' into 222-masthead
2d83f76 
* master: (44 commits)
  Set up color maps for Decanter and refactor main nav colors to refer to color maps (#420)
  Move PR template (#416)
  Fix skiplink target so it doesn't take up space and cause overflow (#415)
  Release 5.0.1 (#417)
  Fix CSS grid gaps not displaying on Edge issue and other minor grid related issues (#413)
  More link fixup (#388)
  forgot to rebuild .js after final lint fixes (#387)
  add openNav, closeNav, openSubnav, closeSubnav events (#379)
  Fix main nav color variants and work with new linting rules (#380)
  add favicon to style guide (#377)
  Adjust linters. (#370)
  update readme.md to match Scott's intro on homepage.md (#371)
  Update README.md (#369)
  move the images we need for the styleguide's homepage into kss-assets (#368)
  Update README.md (#367)
  Update UPGRADE.md (#363)
  package.locl.json file is back (#366)
  Added more to changelog (#364)
  Update homepage.md (#362)
  90 basic webpack (#334)
  ...

# Conflicts:
#	core/scss/components/index.scss
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@JBCSU JBCSU JBCSU approved these changes

@josephgknox josephgknox josephgknox approved these changes

@yvonnetangsu yvonnetangsu yvonnetangsu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sherakama @JBCSU @yvonnetangsu @josephgknox