-
Notifications
You must be signed in to change notification settings - Fork 100
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
Opt-in Atom editor conversion #728
Conversation
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.
Everything looks great to me, although I wish there was a easier way to spot changes when renaming files 😆
Btw, fix the merge conflict on package.json and should be ready to merge 🚀
"eslint-config-prettier": "6.12.0", | ||
"glob": "7.1.6", | ||
"lodash": "^4.17.20", |
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.
Psss, you could have used lodash-es which is tree-shakable hence reduces bundle size. 🍸
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.
Aha! I can never keep track of which Lodash is the right one to use now 😄. Will do.
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.
Hmm, https://www.npmjs.com/package/lodash-es is a year out of date 😕 ... I'll skip this for now, but if it ends up getting regularly published it'd be great to include.
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.
Oh fun, this is an actively discussed topic: https://github.com/lodash/lodash/issues/4879
@@ -52,3 +54,5 @@ export const uniqueFromSources = <T>(...sources: (T | T[] | undefined)[]) => { | |||
|
|||
return Array.from(new Set(items)); | |||
}; | |||
|
|||
export const parseJson = (text: string) => JSON.parse(stripJsonComments(text)); |
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.
After merging this one, should be good to change the JSON.parse
by json5.parse
as stated on #742
const knownMissingSettings = [ | ||
"tslint.alwaysShowRuleFailuresAsWarnings", | ||
"tslint.exclude", | ||
"tslint.ignoreDefinitionFiles", | ||
"tslint.jsEnable", | ||
"tslint.suppressWhileTypeErrorsPresent", | ||
]; |
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.
Clever approach 🍰
@@ -12,8 +12,10 @@ | |||
"dependencies": { | |||
"chalk": "4.1.0", | |||
"commander": "6.1.0", | |||
"cson-parser": "^4.0.5", |
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 get an error npm ERR! 404 Not Found - GET http://my-registry-url/coffeescript/-/coffeescript-1.12.8.tgz
during installation on Windows. It comes from cson-parser
. There are some mentions of this problem, for example at jashkenas/coffeescript/issues/4805, but it's not clear how to fix it while installing Angular Eslint with ng add @angular-eslint/schematics
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.
Hello, can you open a new issue in order to track the error with Angular ESLint + this package? Thank you :)
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.
Thank you, we will look into these and provide a proper fix soon.
PR Checklist
status: accepting prs
Overview
Builds off the excellent work by @MrCube42 (#133 -> #250) to make a general
convertEditorConfigs
function, which callsconvertEditorConfig
for each requested--editor
configuration. Instead of assuming VS Code for all editors, aeditorConfigDescriptors
dependency is added that lists the known file path formats and matches to a known format if possible.Trims down the logic around retrieving the settings keys and such to be a bit more manual in the editor configs. It was a nice thing to build off of but:
Adds two runtime dependencies to
package.json
:cson-parser
: CSON equivalent to the globalJSON
object.lodash
: This was already depended on at runtime so there's no real cost to using its nicemerge
function for merging configs... though this might be removed later in resolving Use a diff writer instead of complete override for JSON editor config conversions #744. I'd like to make that change separately, to keep changes small.Example output snippets from running
node ../tslint-to-eslint-config/bin/tslint-to-eslint-config --editor .vscode/settings.json
: