Skip to content

Conversation

cdcabrera
Copy link
Member

@cdcabrera cdcabrera commented Oct 23, 2017

Description

  • Continuation of non-breaking formatting changes, but for unit tests.
  • Implements a lower form of linting for unit tests.
    • Still in progress, looking at an eslint unit test specific config

Related to PR #644 and issue #620
Closes #646

PR Checklist

  • [n/a] Unit tests are included
  • [n/a] Screenshots are attached (if there are visual changes in the UI)
  • [n/a] A Designer is assigned as a reviewer (if there are visual changes in the UI)
  • [n/a] A CSS rep is assigned as a reviewer (if there are visual changes in the UI)

@beanh66

$scope.nodes = {};
for(var kind in nodeKinds) {

Object.keys(nodeKinds).forEach(function (kind) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Altered this loop since it was linting towards something like.... Can alter it back towards this if we vote this direction

for(var kind in nodeKinds) {
  if (nodeKinds.hasOwnProperty(kind)) {
...

Copy link
Contributor

@amarie401 amarie401 Nov 2, 2017

Choose a reason for hiding this comment

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

I don't have any particular preference. I'm fine with either.

@cdcabrera cdcabrera force-pushed the issue-lint branch 3 times, most recently from 8fdb855 to b9a7430 Compare November 2, 2017 15:33
@cdcabrera cdcabrera changed the title [WIP] Non-Breaking Unit Test Formatting Updates Non-Breaking Unit Test Formatting Updates Nov 2, 2017
no-else-return: 2
no-loop-func: 2
vars-on-top: 2
vars-on-top: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks it's being relaxed. Was that intentional?

@jeff-phillips-18 @dtaylor113

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, was unintentional but if we're good with it, which it sounds like we may be I can leave it be

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

no-else-return: 2
no-loop-func: 2
vars-on-top: 2
vars-on-top: 1
Copy link
Member

Choose a reason for hiding this comment

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

🎉

var eUnFocused2 = page.find("#i2")[0];

$scope.$apply(function(){
$scope.$apply(function () {
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mind removing this rule as well.

eslint.yaml Outdated
space-after-keywords: 2
space-before-blocks: 2
space-before-function-paren: 2
space-before-function-paren: 1
Copy link
Member Author

Choose a reason for hiding this comment

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

Relaxed this to a warning on the main, and turned it off for tests.
But we can go all the way with it @jeff-phillips-18 @dtaylor113 @amarie401 ?

Copy link
Member

Choose a reason for hiding this comment

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

I say remove it completely.

Copy link
Member Author

Choose a reason for hiding this comment

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

cannnnn do!

eslint.yaml Outdated
vars-on-top: 1
no-debugger: 2
no-cond-assign: 2
no-console: 2
Copy link
Member

Choose a reason for hiding this comment

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

Can we change this to a 1? I'd like to see a warning about consoles, not an error.

amarie401
amarie401 previously approved these changes Nov 6, 2017
Update code format for unit tests.
@jeff-phillips-18 jeff-phillips-18 merged commit 3d04a16 into patternfly:master Nov 6, 2017
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.

4 participants