-
Notifications
You must be signed in to change notification settings - Fork 19
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
Move devDependencies
from individual packages to root package.json
#302
base: master
Are you sure you want to change the base?
Conversation
@@ -4,7 +4,6 @@ import path from 'path'; | |||
|
|||
import chalk from 'chalk'; | |||
import eslint from 'eslint'; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? With it, the coding standards should actually fail on this...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspected it was a bug fix in import/order, as all 3 of these imports are external and should not have a blank line between them, particularly the package scope @
character is what was the false positive
Running the tests when including the blank line results in:
Code
import chalk from 'chalk';
import eslint from 'eslint';
import apiFetch from '@wordpress/api-fetch';
Test
/Users/netweb/Code/humanmade/coding-standards/packages/eslint-config-humanmade/fixtures/pass/import-order.js
6:1 error There should be no empty line within import group import/order
✖ 1 problem (1 error, 0 warnings)
1 error and 0 warnings potentially fixable with the `--fix` option.
1 unexpected error!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll take a closer look to try to determine why the rule is failing in this PR, as based on the above it shouldn't be, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good!
Details
This PR moves the
devDependencies
from individual Node.js packages to the rootpackage.json
file, this is because we can run the tests for both packages from the root directory instead of each package directoryActual version bumps to the
devDependencies
will follow in further PRs on MondayAdditional PRs will follow for the
peerDependencies
changesIdeally my initial plan is to release updated versions as v2.0.0 with as few breaking changes as possible
I originally added a Node.js job, but it fails due to a Travis CI issue, so I removed it,will add this back when switching to GitHub Actions
Test Instructions
Install Node.js v16, install dependencies, run tests:
Test Results