Skip to content

Remove *.ts and *.js extension check code duplication#710

Closed
mjbvz wants to merge 8 commits intomicrosoft:masterfrom
mjbvz:remove-ext-check-dup
Closed

Remove *.ts and *.js extension check code duplication#710
mjbvz wants to merge 8 commits intomicrosoft:masterfrom
mjbvz:remove-ext-check-dup

Conversation

@mjbvz
Copy link
Copy Markdown
Contributor

@mjbvz mjbvz commented Feb 26, 2016

Defect
There are currently a number of places that implement logic for checking if a file is Javascript or Typescript based on file extension. This code is hard to read and can get out of sync.

Fix
Created two helper methods to implement these checks. Replace most of the old extension check logic with calls to these helpers instead.

@msftclas
Copy link
Copy Markdown

Hi @mjbvz, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Matt Bierner). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

using System.IO;

namespace Microsoft.NodejsTools {
static class NodejsFileExtensions {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you change the name of this to be something like NodejsFileTypeHelpers? Files named with "Extensions" tend to contain extension methods, so it's a bit overloaded.

@mousetraps
Copy link
Copy Markdown
Contributor

There are also several more locations that are missed with this change that we may as well replace.

👍 after that!

@mousetraps
Copy link
Copy Markdown
Contributor

LGTM, but looks like there are conflicts now (might have been due to #730) - can you update and merge this in?

@mjbvz
Copy link
Copy Markdown
Contributor Author

mjbvz commented Mar 29, 2016

Closing since the line ending change makes merging back in to messy. Will reapply the change and submit another PR.

@mjbvz mjbvz closed this Mar 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants