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

feat(eslint): enable jest rules #29

Merged
merged 13 commits into from
Jun 30, 2020
Merged

feat(eslint): enable jest rules #29

merged 13 commits into from
Jun 30, 2020

Conversation

stevensacks
Copy link
Contributor

Enable jest eslint rules. The plugin was installed but it wasn't being used.

@stevensacks stevensacks requested a review from a team as a code owner April 22, 2020 05:18
@marcusrbrown
Copy link
Contributor

The jest rules are added in test.js, so this patch should target that file.

@stevensacks
Copy link
Contributor Author

Ah, I see. Ok, I'll fix this.

This rule conflicts with co-located tests. A test.js file should be allowed to import from '/index'
@stevensacks
Copy link
Contributor Author

I just pushed a fix for a rule I noticed was conflicting with co-located tests.

@@ -12,6 +12,8 @@
* the License.
*/

/* eslint-disable jest/no-test-callback */
Copy link
Contributor

Choose a reason for hiding this comment

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

well that's embarrassing

might be better to

const path = require('path');
const util = require('util');
const exec = util.promisify(require('child_process').exec);
const packageJson = require('../package.json');
describe('package.json', () => {
  it('should not have peer dep warnings', async () => {
    expect.assertions(2);
    // ex:
    // npm ERR! peer dep missing: eslint@<2.3.0, required by babel-eslint@5.0.4
    const { stdout, stderr } = exec('npm ls', { cwd: path.resolve(__dirname, '../') });
    // if there are no complaints from npm, stdout is null
    expect(stdout || '').not.toMatch('peer dep missing');
    expect(stderr || '').not.toMatch('peer dep missing');
  });

than disabling rules in our own repository/project?

Copy link
Contributor Author

@stevensacks stevensacks May 11, 2020

Choose a reason for hiding this comment

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

It's not my code and I didn't immediately know how to make it compliant, so I disabled the rule as a quick solution. If you know how to fix it, please do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@PixnBits PixnBits left a comment

Choose a reason for hiding this comment

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

prettier also changed the license headers, left suggestions to revert, hoping that makes it easy to fix and merge quickly 🤞

@PixnBits
Copy link
Contributor

I think these were intended to be active but were overlooked? If that's the case, @10xLaCroixDrinker do you think this might be a fix? Adding breaking rules might be a major version feature? 🤔

@10xLaCroixDrinker
Copy link
Member

I think these were intended to be active but were overlooked? If that's the case, @10xLaCroixDrinker do you think this might be a fix? Adding breaking rules might be a major version feature? 🤔

Arent we bundling all these PRs into a major release anyway?

@JAdshead JAdshead changed the base branch from master to feature/v13 June 30, 2020 21:21
@JAdshead JAdshead merged commit 35f6221 into americanexpress:feature/v13 Jun 30, 2020
JAdshead pushed a commit that referenced this pull request Jul 16, 2020
oneamexbot added a commit that referenced this pull request Jul 16, 2020
# [13.0.0](v12.2.0...v13.0.0) (2020-07-16)

### Bug Fixes

* **eslint:** disable prevent abbreviations ([#33](#33)) ([6647bef](6647bef))

### Features

* **eslint:** configuration ([#30](#30)) ([6229d4a](6229d4a))
* **eslint:** disable function scoping ([#35](#35)) ([0a0c5bb](0a0c5bb))
* **eslint:** disable prefer default export ([#26](#26)) ([45b88d3](45b88d3))
* **eslint:** enable jest rules ([#29](#29)) ([846bbf7](846bbf7))
* **eslint:** react/jsx quality of life rules ([#24](#24)) ([facff92](facff92))
* **js:** additional js rules ([58df834](58df834))
* **prettier:** add ([#45](#45)) ([00cecaf](00cecaf))
* **unicorn:** update and seperate rules ([#43](#43)) ([940eb66](940eb66))
* **unicorn/consistent-function-scoping:** disable ([#42](#42)) ([54dd5a3](54dd5a3))

### BREAKING CHANGES

* **unicorn:** major update to unicorn
* **js:** no-lonely-if errors
* **js:** no-return-assign, errors
* **js:** prefer-object-spread, errors
* **js:** no-bitwise, errors
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.

6 participants