Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Revamped configs into 'core', 'latest', and (legacy) 'recommended' #809

Merged
merged 7 commits into from
Feb 19, 2019
Merged

Revamped configs into 'core', 'latest', and (legacy) 'recommended' #809

merged 7 commits into from
Feb 19, 2019

Conversation

JoshuaKGoldberg
Copy link

PR checklist

Overview of change:

Those three config files now exist as .json under /configs; they're copied to dist/build/configs. recommended.json becomes tslint.json as well.

Is there anything you'd like reviewers to focus on?

Does this look like a reasonable way to manage configurations for foreseeable future?

Those three config files now exist as `.json` under `/configs`; they're copied to `dist/build/configs`. `recommended.json` becomes `tslint.json` as well.
@VincentLanglet
Copy link

VincentLanglet commented Feb 7, 2019

@JoshuaKGoldberg The same way tslint expose a tslint:all config, you may create a tslint-microsoft-config:all config.

And I, personally, would loved a tslint-microsoft-config:a11y but i'm may be the only one.

@IllusionMH IllusionMH self-assigned this Feb 7, 2019
@JoshuaKGoldberg
Copy link
Author

JoshuaKGoldberg commented Feb 9, 2019

:all

Yes!! Great idea @VincentLanglet, will do.

Edit: looking through, it seems like latest.ts is already what :all would be. Are there other rules/options missing?

:a11y

Also strongly in support of this! But - I think it would be better, long-term, to move those rules to tslint-react, as it's been confusing to many users where different contributed rules come from & why. Will file issues there.

@VincentLanglet
Copy link

:all

Yes!! Great idea @VincentLanglet, will do.

Edit: looking through, it seems like latest.ts is already what :all would be. Are there other rules/options missing?

I didn't have rules in mind. I was just thinking it was the same naming than tslint configs :

  • recommended is a subset of all rules, minor version doesn't add breaking changes.
  • latest is the recommended rules for the next major version, but still a subset of all rules.
  • all is all.

If you're going to do:

  • stable every rules but without breaking changes for minor version.
  • latest every rules. Will be the stable of the next major version.

You may rename the latest to all

:a11y

Also strongly in support of this! But - I think it would be better, long-term, to move those rules to tslint-react, as it's been confusing to many users where different contributed rules come from & why. Will file issues there.

It's not a bad idea to move those rules to tslint-react. But it's not the same maintainers (Microsoft vs palantir) ; i don't know the needs and consequences.
Anyway it will be a big breaking change. The :a11y can be a first step.

Copy link
Contributor

@IllusionMH IllusionMH left a comment

Choose a reason for hiding this comment

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

There are couple fixes that are required for code and couple of questions.

According to README.md - it stable configuration will be changes once before major release. Therefore, it makes sense to make stable.json as static file and automatically generate latest.json.
With current approach - it will be really easy to forget to add rule to latest.json and it will slip into stable.json potentially breaking promise of update it only on major versions.

build-tasks/templates/stable.json.template.js Outdated Show resolved Hide resolved
build-tasks/templates/stable.json.template.js Outdated Show resolved Hide resolved
build-tasks/generate-stable-config.js Outdated Show resolved Hide resolved
build-tasks/generate-stable-config.js Outdated Show resolved Hide resolved
build-tasks/copy-config-json.js Outdated Show resolved Hide resolved
configs/recommended.json Outdated Show resolved Hide resolved
configs/recommended.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@IllusionMH IllusionMH left a comment

Choose a reason for hiding this comment

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

Found second rule that was added after 6.0.0 and shouldn't be in stable.json

configs/stable.json Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the PR: Waiting for Author Changes have been requested that the pull request author should address. label Feb 18, 2019
@JoshuaKGoldberg
Copy link
Author

JoshuaKGoldberg commented Feb 18, 2019

Thanks for the feedback you two! I agree with what looks like everything that was suggested. 😄

Changed the names of the configurations to be:

  • recommended.json: aliased by "extends": ["tslint-microsoft-contrib/recommended"]
  • latest.json: aliased by "extends": ["tslint-microsoft-contrib/latest"]
  • legacy.json: aliased by "extends": ["tslint-microsoft-contrib"], with a note that that will eventually be an alias for recommended.json.

Thoughts?

Rule configs are, for now, still in configs/ and copied to dist/build/, but once the repository changes to in-place (#825), they can be moved to the root directory.

Filed palantir/tslint-react#202 for moving a11y rules into tslint-react. If that doesn't see traction any time soon (it looks like that repository hasn't had much maintenance since June 2018...), we can add the equivalent here. Let's discuss that later on?

@JoshuaKGoldberg JoshuaKGoldberg added PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! and removed PR: Waiting for Author Changes have been requested that the pull request author should address. labels Feb 18, 2019
Copy link
Contributor

@IllusionMH IllusionMH left a comment

Choose a reason for hiding this comment

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

Thanks! After latest changes PR looks great and can be merged as is.
Only one minor comment that I'm not sure should be addressed.

src/utils/ExtendedMetadata.ts Outdated Show resolved Hide resolved
@IllusionMH IllusionMH added PR: Waiting for Author Changes have been requested that the pull request author should address. and removed PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! labels Feb 19, 2019
@JoshuaKGoldberg
Copy link
Author

Whoohoo, thanks for the reviews @IllusionMH & @VincentLanglet! This'll be available in the next release.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 1708055 into microsoft:master Feb 19, 2019
@JoshuaKGoldberg JoshuaKGoldberg deleted the configs branch February 19, 2019 13:04
@IllusionMH IllusionMH added this to the 6.1.0-beta milestone Feb 19, 2019
@olevett olevett mentioned this pull request Feb 25, 2019
1 task
apawast pushed a commit to lupine86/tslint-microsoft-contrib that referenced this pull request Feb 26, 2019
…icrosoft#809)

* Revamped configs into 'core', 'latest', and (legacy) 'recommended'

Those three config files now exist as `.json` under `/configs`; they're copied to `dist/build/configs`. `recommended.json` becomes `tslint.json` as well.

* Normalized trailing commas; added back .gitignore fields

* Moved void-zero to latest.json

* Moved to exporting root-level configs

* Switched to auto-generating latest.json

* Aligned naming with TSLint's recommended -> latest

* Update ExtendedMetadata.ts
:qa!

rebasing
:wq

:qa!
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR: Waiting for Author Changes have been requested that the pull request author should address.
Projects
None yet
3 participants