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

Change double-quote hygiene rule to tslint rule #6514

Merged
merged 2 commits into from Aug 5, 2019

Conversation

Charles-Gagnon
Copy link
Contributor

No description provided.

@@ -351,6 +338,17 @@ function hygiene(some) {
input = some;
}

const tslintSqlConfiguration = tslint.Configuration.findConfiguration('tslint-sql.json', '.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Youy don't need to add this to the hygiene run, this will automatically run in the tslint check if you have the rule in the tslint directory and have the rule in the tslint file.

Copy link
Contributor

Choose a reason for hiding this comment

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

If your goal is to limit the scope of the files included for this rule, I would just modify the tslint gulp task to run a second pass with just our lint file, rather than putting this into the hygiene task.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could also see if vscode has interest in taking this rule, since they have the guidance and when they change code they general update any code that doesn't follow the guidance, they probably just don't see this as big enough of a problem to dedicate time to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I didn't add it to the tslint.json because that runs on all files and I didn't want to fix up the vscode ones since that'd just result in a bunch of changes that might cause issues merging down the line. Hence the creation of the new tslint file that will run on just the sql code. (I'll update it later to run on our extensions too - I just didn't want to do the changes now since the fix logic isn't working for some reason so I need to investigate that further and doing the replacements manually is a pain)

And I still want it to run as part of hygiene so that the precommit hook catches it - since my current understanding if I'm reading the configuration correctly is that it only runs the hygiene task as a precommit hook and thus if someone ignores the GCI failing (which people do quite a bit) they might not catch the issue.

I'll follow up with seeing if vscode will take this but for now I figure we can get it in our repo and then remove it later if they decide to take it.

@Charles-Gagnon Charles-Gagnon merged commit 8a6dc02 into master Aug 5, 2019
@Charles-Gagnon Charles-Gagnon deleted the chgagnon/tslintLocalizeRule branch August 5, 2019 16:48
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.

None yet

2 participants